-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_node_config: make top level version_constant_overrides optional #10286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 11-19-apollo_gateway_insert_optional_versioned_constant_overrides_to_batcher
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)
crates/apollo_deployments/resources/app_configs/versioned_constants_overrides_config.json line 2 at r1 (raw file):
{ "versioned_constants_overrides.#is_none": true,
This sets all of these params as optional in future deployments. Is that intentional?
Code quote:
truecrates/apollo_deployments/resources/app_configs/versioned_constants_overrides_config.json line 7 at r1 (raw file):
"versioned_constants_overrides.max_recursion_depth": 50, "versioned_constants_overrides.validate_max_n_steps": 1000000 }
Please keep the new line 🙏
Code quote:
crates/apollo_node_config/src/node_config.rs line 163 at r1 (raw file):
"gateway_config.stateful_tx_validator_config.versioned_constants_overrides.#is_none", ]), ),
I think you get this "for free" from the following, could you please check?
let mut common_execution_config = generate_struct_pointer(
"versioned_constants_overrides".to_owned(),
&VersionedConstantsOverrides::default(),
set_pointing_param_paths(&[
"batcher_config.block_builder_config.versioned_constants_overrides",
"gateway_config.stateful_tx_validator_config.versioned_constants_overrides",
]),
);
If the "versioned_constants_overrides.#is_none": is added to the config schema without this chunk then we can just drop it
Code quote:
(
ser_pointer_target_param(
"versioned_constants_overrides.#is_none",
&false,
"Flag to disable versioned_constants_overrides for all components.",
),
set_pointing_param_paths(&[
"batcher_config.block_builder_config.versioned_constants_overrides.#is_none",
"gateway_config.stateful_tx_validator_config.versioned_constants_overrides.#is_none",
]),
),0653c5f to
3436445
Compare
74b46eb to
21939ad
Compare
|
Previously, Itay-Tsabary-Starkware wrote…
Without this chunk: Those lines do not appear in the config_schema: And the lines of the other is_none config become independent (without pointerTarget): |
3436445 to
905a1f5
Compare
|
Previously, Itay-Tsabary-Starkware wrote…
Done |
|
Previously, Itay-Tsabary-Starkware wrote…
Yes, please verify me here. @TzahiTaub |
905a1f5 to
ccd11b8
Compare
21939ad to
c8e15be
Compare
TzahiTaub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TzahiTaub reviewed 2 of 3 files at r2, all commit messages.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @itamar-starkware and @Itay-Tsabary-Starkware)
crates/apollo_deployments/resources/app_configs/versioned_constants_overrides_config.json line 2 at r1 (raw file):
Previously, itamar-starkware wrote…
Yes, please verify me here. @TzahiTaub
Correct
Yoni-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yoni-Starkware reviewed all commit messages.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @itamar-starkware and @Itay-Tsabary-Starkware)
c8e15be to
ca29fcc
Compare
ccd11b8 to
c47330a
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShahakShama reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/apollo_deployments/resources/app_configs/replacer_versioned_constants_overrides_config.json line 2 at r3 (raw file):
{ "versioned_constants_overrides.#is_none": true,
@Itay-Tsabary-Starkware please make sure that this is false for potc
TzahiTaub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TzahiTaub reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @itamar-starkware and @Itay-Tsabary-Starkware)

Added support for properly disabling versioned constants overrides in configuration.
What changed?
ser_optional_sub_configfunction to handle optional configuration sections#is_noneflag in the config schema to explicitly disable versioned constants overridesWhy make this change?
This change improves the configuration system by providing a cleaner way to disable optional configuration sections. Previously, optional versioned constants were handled by unwrapping with a default value, which didn't properly represent when the section was intentionally disabled. The new approach with the
#is_noneflag makes it explicit when a configuration section should be disabled.