-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_config: update config map by pointers skip optional values #10510
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
b0808ae to
3f28fa2
Compare
dfb5376 to
4741ddb
Compare
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 2 files reviewed, 1 unresolved discussion (waiting on @itamar-starkware and @TzahiTaub)
crates/apollo_config/src/dumping.rs line 99 at r1 (raw file):
pointer_prefixes: HashSet<ParamPath>, is_none_default: bool, is_none_description: &str,
I suggest avoid explicitly defining these two:
- Change
default_instance: &Tto be of typeOption, and derive the default value from that - Use the default optional description of optional parameter flags, as defined in
fn ser_is_param_none(). Bonus points if you can use that fn in the implementation as well.
Code quote:
is_none_default: bool,
is_none_description: &str,
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 2 files reviewed, 2 unresolved discussions (waiting on @itamar-starkware and @TzahiTaub)
crates/apollo_config/src/loading.rs line 225 at r1 (raw file):
continue; } }
AFAIU there is an implicit assumption here that the X.is_none entries precede their respective Xentries.
This holds probably due to the fact we're using BTrees and the fact we defined our separator, but imo the implementation shouldn't count on it (otherwise the arbitrary placeholder becomes non-arbitrary).
What about following the logic used for handling the "non-struct" pointers?
IIRC it's somehing like:
- add all entries with whatever values they have
- find all optional param flags (the
is_nonemarkers) with atrueflag - delete any key whose prefix is the above
PTAL at
sequencer/crates/apollo_config/src/loading.rs
Lines 96 to 116 in 4741ddb
| // Address optional None value discrepancies: | |
| // 1. Find None (null) values in the config entries. | |
| let null_config_entries = values_map | |
| .iter() | |
| .filter(|(_, value)| value.is_null()) | |
| .map(|(key, _)| key.clone()) | |
| .collect::<HashSet<_>>(); | |
| // 2. Filter out None-value keys in the loaded config entries. | |
| keys_only_in_loaded_config.retain(|item| !null_config_entries.contains(item.as_str())); | |
| // 3. Filter out None-value keys in the schema entries. | |
| let optional_param_suffix = format!("{FIELD_SEPARATOR}{IS_NONE_MARK}"); | |
| keys_only_in_schema.retain(|item| { | |
| // Consider a schema key only if both: | |
| // - it does NOT start with any prefix in `null_config_entries` (i.e., a None value) | |
| // - it does NOT end with the optional param suffix (i.e., a None value indicator) | |
| let has_prefix = null_config_entries.iter().any(|prefix| item.starts_with(prefix)); | |
| let has_suffix = item.ends_with(optional_param_suffix.as_str()); | |
| !has_prefix && !has_suffix | |
| }); |
Code quote:
// 1) Resolve flags first: these populate "<prefix>.#is_none" into the config_map.
if param_path.ends_with(&format!("{FIELD_SEPARATOR}{IS_NONE_MARK}")) {
let Some(target_value) = config_map.get(target_param_path) else {
return Err(ConfigError::PointerTargetNotFound {
target_param: target_param_path.to_owned(),
});
};
config_map.insert(param_path.to_owned(), target_value.clone());
continue;
}
// 2) If the optional prefix is already marked as disabled, skip resolving nested pointers.
if let Some((prefix, _)) = param_path.rsplit_once('.') {
if config_map.get(&format!("{prefix}{FIELD_SEPARATOR}{IS_NONE_MARK}"))
== Some(&json!(true))
{
continue;
}
}
Added support for optional struct pointers in the Apollo configuration system.
What changed?
generate_optional_struct_pointerfunction indumping.rsthat extends the existinggenerate_struct_pointerfunctionality to handle optional structs#is_noneflag to indicate whether the optional struct is present or notupdate_config_map_by_pointersfunction inloading.rsto:#is_noneflags firstWhy make this change?
This change improves the configuration system by adding proper support for optional structs. It allows users to explicitly mark a struct as absent using the
#is_noneflag, and prevents unnecessary resolution of nested pointers when the parent struct is disabled, which improves efficiency and clarity in the configuration handling.