feat: override NetworkConfig from JSON config #8871
Conversation
chain/network/src/config_json.rs
Outdated
| pub network_config_overrides: NetworkConfigOverrides, | ||
| } | ||
|
|
||
| /// Override values from NetworkConfig. |
There was a problem hiding this comment.
Please comment why specifically these values.
chain/network/src/config.rs
Outdated
| @@ -293,7 +355,7 @@ impl NetworkConfig { | |||
| }, | |||
| event_sink: Sink::null(), | |||
| }; | |||
| Ok(this) | |||
| Ok(Self::override_config(cfg.experimental.network_config_overrides, this)) | |||
There was a problem hiding this comment.
Self::foo(this) is semantically equivalent to calling this.foo().
chain/network/src/config.rs
Outdated
| this.routing_table_update_rate_limit | ||
| }, | ||
|
|
||
| node_addr: this.node_addr, |
There was a problem hiding this comment.
I'd like to avoid initialization of the fields as much as possible.
Would it be an Option (pun intended :) ) to change this function to accept a &mut Config and modify the fields that need to be overridden? That would let you not mention the fields that can already be set via a config file.
There was a problem hiding this comment.
I also considered it. It can be done by passing self and only overriding the appropriate values.
The reason that I went for this approach is, in case you add a new field to NetworkConfig, you will get an warning in this function and you will be able to decide if you want to expose it in the JSON config or not.
There was a problem hiding this comment.
That is a reasonable approach.
However, when someone adds a new field to NetworkConfig, why wouldn't they simply add a field here? How can they be made aware that each new field needs to be configurable, either directly, or via this emergency mechanism?
There was a problem hiding this comment.
why wouldn't they simply add a field here
Authors can overlook this override function.
How can they be made aware that each new field needs to be configurable
They will be made aware by the rustc error saying that there is a missing field in this instantiation and they will read the comment. At this point they will decide if they want to keep the value or overriding it by going to NetworkConfigOverrides and adding the field there.
chain/network/src/config.rs
Outdated
| @@ -168,6 +168,68 @@ pub struct NetworkConfig { | |||
| } | |||
|
|
|||
| impl NetworkConfig { | |||
| /// Here you can override the NewtworkConfig with values for the JSON config. | |||
There was a problem hiding this comment.
you can
Technically, one does it by setting values in config.network.experimental.
Suggestion:
Overrides values of NetworkConfig that normally need not to be touched
VanBarbascu
force-pushed
the
config_overrides
branch
from
4876b5a
to
3fbe8a2
Compare
VanBarbascu
force-pushed
the
config_overrides
branch
from
3fbe8a2
to
9706e75
Compare
chain/network/src/config.rs
Outdated
| /// Overrides values of NetworkConfig with values for the JSON config. | ||
| /// We need all the values from NetworkConfig to be configurable. | ||
| /// We need this in case of emergency. It is faster to change the config than to recompile. | ||
| fn override_config(mut self, overrides: crate::config_json::NetworkConfigOverrides) -> Self { |
There was a problem hiding this comment.
[stylistic personal preference]
fn override(mut self) -> Self {} works, but I'd go for fn override(&mut self) {} to make the callsite less confusing.
x.override();
Ok(x)
seems clearer to me than
Ok(x.override())
9706e75
to
9e39cf6
Compare
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig. The JSON config override is done before the CLI overrides.
VanBarbascu
force-pushed
the
config_overrides
branch
from
9e39cf6
to
4f8f4fc
Compare
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig. The JSON config override is done before the CLI overrides.
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig. The JSON config override is done before the CLI overrides.
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig. The JSON config override is done before the CLI overrides.
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig. The JSON config override is done before the CLI overrides.
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig. The JSON config override is done before the CLI overrides.
Added config.experimental.network_config_overrides field. It contains the overrides for the currently default values from NetworkConfig.
The JSON config override is done before the CLI overrides.