(config) handle skip_subsequent_node in legacy Delta-xDS #42650
Conversation
Signed-off-by: antoniovleonti <[email protected]>
|
/assign @adisuissa |
Signed-off-by: antoniovleonti <[email protected]>
antoniovleonti
changed the title
Signed-off-by: antoniovleonti <[email protected]>
Signed-off-by: antoniovleonti <[email protected]>
| - area: config | ||
| change: | | ||
| Added support for `:ref:`set_node_on_first_message_only | ||
| <envoy_api_field_core.ApiConfigSource.set_node_on_first_message_only>` to Delta-xDS |
There was a problem hiding this comment.
Please clarify the runtime flag to disable the new behavior.
| void updateSubscriptionInterest(const absl::flat_hash_set<std::string>& cur_added, | ||
| const absl::flat_hash_set<std::string>& cur_removed); | ||
| void setMustSendDiscoveryRequest() { must_send_discovery_request_ = true; } | ||
| void setDynamicContextChanged() { |
There was a problem hiding this comment.
High-level question: why is there a need to know whether a dynamic context has been modified or not? Should the must_send_discovery_request_ cover that?
(the way I understood this code was: the context change is on a higher level of abstraction than the gRPC-state. The gRPC-state is only interested in whether it needs to sent a discovery request or not). [in other words who is using setMustSendDiscoveryRequest() if not the dynamic context change]
| if (sub->second->sub_state_.dynamicContextChanged() || | ||
| !Runtime::runtimeFeatureEnabled( | ||
| "envoy.reloadable_features.legacy_delta_xds_skip_subsequent_node") || | ||
| !skip_subsequent_node_ || !node_sent_in_current_stream_) { |
There was a problem hiding this comment.
nit: can there be a bool flag (something like populate_node_in_request), to make it a bit more readable?
| DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, | ||
| const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, | ||
| XdsConfigTrackerOptRef xds_config_tracker); | ||
| Event::Dispatcher& dispatcher, XdsConfigTrackerOptRef xds_config_tracker); |
There was a problem hiding this comment.
I find it interesting that all the tests pass with the unified_mux implementation (no changes were needed there AFAICT). Can you double check whether that is working as expected?
2 participants
Commit Message: (config) handle skip_subsequent_node in legacy Delta-xDS
Additional Description:
Added a runtime guard since someone may erroneously have this set for Delta-xDS and be broken when it suddenly starts working.
Risk Level: low
Testing: unit / integration tests added
Docs Changes: none
Release Notes: changelog updated
Platform Specific Features:
[Optional Runtime guard:]
Fixes #14976
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]