refactor: Split configs in server and service #671

flub · 2023-01-03T16:53:46Z

We used to use the same config for the service and server (aka the binary). This is confusing when creating configs to use with e.g. iroh-one, iroh-embed or iroh-share because some fields are not used.

This splits off the config structs to avoid this problem, services now have a Config and binaries a ServerConfig. This allows creating the services standalone without all the baggage a server needs.

While this isn't many fields yet, this will get worse as we add more features (this is split off from another PR where this seemed useful).

Todo

iroh-store
iroh-p2p
iroh-gateway

We used to use the same config for the service and server (aka the binary). This is confusing when creating configs to use with e.g. iroh-one, iroh-embed or iroh-share because some fields are not used. This splits off the config structs to avoid this problem, services now have a Config and binaries a ServerConfig. This allows creating the services standalone without all the baggage a server needs. While this isn't many fields yet, this will get worse as we add more features (this is split off from another PR where this seemed useful).

flub · 2023-01-03T17:09:02Z

iroh-store/src/config.rs

+// TODO: I'd prefer to include [`Config`] under the `store` field like iroh-one does.  But
+// that's a backwards incompatible change.


@Arqu I guess you're the main person that uses these config files? What are your thoughts here?

For the p2p and gateway services this duplication is painful, it would be a lot nicer if we could break backwards compatibility of the config file formats.

You should break it, there's no real dependency on them yet. Better break them now than later.

flub · 2023-01-03T17:50:04Z

iroh-share/src/p2p_node.rs

@@ -173,10 +172,6 @@ impl P2pNode {
        let store_config = iroh_store::Config {
            path: db_path.to_path_buf(),
            rpc_client: rpc_store_client_config,
-            metrics: iroh_metrics::config::Config {
-                tracing: false, // disable tracing by default


This is proof that the current state of things is weird. This did nothing IIUC.

dignifiedquire · 2023-01-04T10:30:30Z

iroh-gateway/src/config.rs

+/// This is the configuration which the gateway server binary needs to run.  This is a
+/// superset from the configuration needed by the gateway service, which can also run
+/// integrated into another binary like iroh-one, iroh-share or iroh-embed.
+// TODO: I'd prefer to include [`Config`] under the `gateway` field liek iroh-one does.  But


I think this should be done before merging, to avoid the duplication, don’t be afraid to break apis

Yeah, feel free to break this, there's nobody really depending on it yet.

Arqu

From a prue code perspective this is 👍
However this "works" for the metrics piece more by coincidence than design at this point.
The reason each had their own metrics directly planted was that different services used to run a different feature set of the metrics crate, which would initialize the metrics just for that service.
This kinda nukes that notion completely. But it was not doing that anyways since the Cargo.toml refactors because it's now from a global workspace and I believe it just builds all the features regardless.

Functionally this is fine, it just wastes a little bit more resources on the metrics side. Not a ton though. I'll spend some time thinking about this.

Arqu · 2023-01-04T12:12:37Z

iroh-gateway/src/config.rs

+/// This is the configuration which the gateway server binary needs to run.  This is a
+/// superset from the configuration needed by the gateway service, which can also run
+/// integrated into another binary like iroh-one, iroh-share or iroh-embed.
+// TODO: I'd prefer to include [`Config`] under the `gateway` field liek iroh-one does.  But


Yeah, feel free to break this, there's nobody really depending on it yet.

Arqu · 2023-01-04T12:18:16Z

iroh-store/src/config.rs

+// TODO: I'd prefer to include [`Config`] under the `store` field like iroh-one does.  But
+// that's a backwards incompatible change.


You should break it, there's no real dependency on them yet. Better break them now than later.

flub · 2023-01-04T12:52:50Z

From a prue code perspective this is +1 However this "works" for the metrics piece more by coincidence than design at this point. The reason each had their own metrics directly planted was that different services used to run a different feature set of the metrics crate, which would initialize the metrics just for that service. This kinda nukes that notion completely.

But it was not doing that anyways since the Cargo.toml refactors because it's now from a global workspace and I believe it just builds all the features regardless.

Hum, this should not be the case. If you look at iroh-store/Cargo.toml you'll find iroh-metrics = { workspace = true, features = ["store"] }. So if you only build the iroh-store crate the only feature built for iroh-metrics is the store feature. If you however also build other crates at the same time I believe the iroh-metrics crate will be built with the combination of all the features requested. This behaves exactly the same as before the workspace.dependencies refactor.

Functionally this is fine, it just wastes a little bit more resources on the metrics side. Not a ton though. I'll spend some time thinking about this.

This adds a StoreConfig section to remove duplication.

lets remove that struct from the other places too

flub · 2023-01-04T18:17:40Z

After taking the wrong approach to resolve the TODOs and going astray for a while I've now tiedied this up. So the ServerConfig structs use the Config structs directly which is much neater and easier to follow.

PTAL

flub commented Jan 3, 2023

View reviewed changes

flub added 2 commits January 3, 2023 18:21

p2p

0858835

gateway

3518561

flub commented Jan 3, 2023

View reviewed changes

flub marked this pull request as ready for review January 3, 2023 17:52

flub requested review from ramfox and Arqu January 3, 2023 17:52

dignifiedquire reviewed Jan 4, 2023

View reviewed changes

Arqu previously approved these changes Jan 4, 2023

View reviewed changes

remove duplication between store Config and ServerConfig

6e91e09

This adds a StoreConfig section to remove duplication.

flub dismissed Arqu’s stale review via 6e91e09 January 4, 2023 14:57

flub added 8 commits January 4, 2023 16:17

p2p config split-ish

46ae1da

gateway, but without the stupid intermediate struct

b06c476

lets remove that struct from the other places too

more sane approach for p2p too

d11e74a

and store, let's see what this looks like

14b513b

maybe don't explode?

2b08d3f

needless diff

7eddc0b

fix test

4bae9f6

Merge branch 'main' into flub/split-configs

72df6d4

flub requested review from dignifiedquire and Arqu January 4, 2023 18:17

dignifiedquire approved these changes Jan 4, 2023

View reviewed changes

flub merged commit a0db860 into main Jan 4, 2023

flub deleted the flub/split-configs branch January 4, 2023 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Split configs in server and service #671

refactor: Split configs in server and service #671

flub commented Jan 3, 2023 •

edited

Loading

flub Jan 3, 2023

flub Jan 3, 2023 •

edited

Loading

Arqu Jan 4, 2023

flub Jan 3, 2023

dignifiedquire Jan 4, 2023

Arqu Jan 4, 2023

Arqu left a comment

Arqu Jan 4, 2023

Arqu Jan 4, 2023

flub commented Jan 4, 2023

flub commented Jan 4, 2023

		// TODO: I'd prefer to include [`Config`] under the `store` field like iroh-one does. But
		// that's a backwards incompatible change.

refactor: Split configs in server and service #671

refactor: Split configs in server and service #671

Conversation

flub commented Jan 3, 2023 • edited Loading

Todo

flub Jan 3, 2023

Choose a reason for hiding this comment

flub Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Arqu Jan 4, 2023

Choose a reason for hiding this comment

flub Jan 3, 2023

Choose a reason for hiding this comment

dignifiedquire Jan 4, 2023

Choose a reason for hiding this comment

Arqu Jan 4, 2023

Choose a reason for hiding this comment

Arqu left a comment

Choose a reason for hiding this comment

Arqu Jan 4, 2023

Choose a reason for hiding this comment

Arqu Jan 4, 2023

Choose a reason for hiding this comment

flub commented Jan 4, 2023

flub commented Jan 4, 2023

flub commented Jan 3, 2023 •

edited

Loading

flub Jan 3, 2023 •

edited

Loading