LEDS: base implementation #17869
LEDS: base implementation #17869
Conversation
Signed-off-by: Adi Suissa-Peleg <[email protected]>
source/common/upstream/leds.h
Outdated
| // unsuccessful). | ||
| const UpdateCb callback_; | ||
| // Once the endpoints of the locality are updated, it is considered active. | ||
| bool active_{false}; |
There was a problem hiding this comment.
"active" confused me on first read. I might recommend initial_update_attempt_complete_ or something like that with matching accessor above.
source/common/upstream/leds.cc
Outdated
| callback_(); | ||
| } | ||
|
|
||
| bool LedsSubscription::validateUpdateSize(int num_resources) { return num_resources > 0; } |
There was a problem hiding this comment.
nit: I don't think this function adds much. I would just inline it.
source/common/upstream/leds.cc
Outdated
| continue; | ||
| } | ||
|
|
||
| auto resource_it = endpoint_entry_map_.find(added_resource_name); |
There was a problem hiding this comment.
perf nit: I think you can potentially avoid a double lookup on add by just using [] lookup and getting a reference and see if there is a valid iterator or not (you might need some wrapper which initializes the iterator to end()). Not sure if this is worth it or not.
There was a problem hiding this comment.
As suggested in another comment, I've modified it from a list and map to a map only, so I think this comment is outdated.
source/common/upstream/leds.cc
Outdated
| envoy::config::endpoint::v3::LbEndpoint lb_endpoint = | ||
| dynamic_cast<const envoy::config::endpoint::v3::LbEndpoint&>( | ||
| added_resource.get().resource()); |
There was a problem hiding this comment.
nit: you can lift this to above and then use in both places.
I suppose in both cases you need to copy but not sure if there would be any perf benefit to trying to move the proto or not.
source/common/upstream/leds.h
Outdated
| // Fetch all the endpoints in the locality subscription. | ||
| const LbEndpointList& getEndpoints() const { return endpoints_; } |
There was a problem hiding this comment.
Thinking to future changes, do you need this list at all? Could you just return the map and have the consumer iterate over that instead?
There was a problem hiding this comment.
Done.
The idea to only return the list was to abstract over the resource names, as they are not really needed by the consumer, but it may be relevant for future changes, so modified to returning the map instead.
source/common/upstream/leds.cc
Outdated
| if (!Config::XdsResourceIdentifier::hasXdsTpScheme(removed_resource_name)) { | ||
| ENVOY_LOG(warn, | ||
| "Received non-glob collection resource name for an unsubscribed resource {} in " | ||
| "LEDS update for cluster: {}. Skipping entry.", | ||
| removed_resource_name, cluster_name_); |
There was a problem hiding this comment.
I'm confused by this error checking and the error message here and below.
Don't you want to actually check to see if the resource name is in the glob prefix that you subscribed to? This is just checking that it starts with xdstp://, which while seems valid, I don't think matches the error message?
There was a problem hiding this comment.
Removed.
The motivation was to verify that only resources matching the glob collection are part of the added/removed resources lists. I've taken another look, and this is already being done in the watch-map code, so no need for this verification here.
Signed-off-by: Adi Suissa-Peleg <[email protected]>
Signed-off-by: Adi Suissa-Peleg <[email protected]>
|
Thanks LGTM but I think needs a tidy fix. /wait |
Signed-off-by: Adi Suissa-Peleg <[email protected]>
Signed-off-by: Adi Suissa-Peleg <[email protected]>
Commit Message: LEDS - base implementation
Additional Description:
The implementation of the main logic behind the LEDS protocol.
This is not integrated into
ClusterLoadAssignmentprocessing (not plumbed in yet).Risk Level: Low.
Testing: Unit test (integration tests will be added in future PR, when it is integrated).
Docs Changes:
Release Notes:
Platform Specific Features:
Part of #10373
Signed-off-by: Adi Suissa-Peleg [email protected]