Passing priority sets to loadbalancers (still unused for picking) #2154
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
force-pushed
the
subset_lb_pri
branch
from
386b9e6
to
a45f341
Compare
|
Due to risk of change can we please hold merging until we tag 1.5.0. Thank you. Will also review later today or tomorrow morning. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| } | ||
|
|
||
| HostSetPtr SubsetLoadBalancer::PrioritySubsetImpl::createHostSet(uint32_t priority) { | ||
| RELEASE_ASSERT(priority < original_priority_set_.hostSetsPerPriority().size()); |
There was a problem hiding this comment.
nit: This should probably be a normal ASSERT
There was a problem hiding this comment.
Happy to downgrade it in my follow-up but we'll crash on the next line regardless. I just find crashes of "you have the wrong size" easier than "there's some invalid pointer dereference".
There was a problem hiding this comment.
It's not a big deal either way, but in the rest of the code this type of check would most likely be a normal debug assert.
| @@ -234,6 +232,9 @@ ClusterImplBase::ClusterImplBase(const envoy::api::v2::Cluster& cluster, | |||
| Ssl::ContextManager& ssl_context_manager, bool added_via_api) | |||
| : runtime_(runtime), info_(new ClusterInfoImpl(cluster, source_address, runtime, stats, | |||
| ssl_context_manager, added_via_api)) { | |||
| // Create the default (empty) priority set before registering callbacks to | |||
| // avoid getting an update the first time it is accessed. | |||
There was a problem hiding this comment.
I actually had a question about this. I notice that in getOrCreate() you call runUpdateCallbacks() not matter what. Why is that the case? Could you add a comment there in a follow up?
There was a problem hiding this comment.
Hahaha, copy-paste from my follow-up HostSet& PrioritySetImpl::getOrCreateHostSet(uint32_t priority) {
// TODO(alyssawilk) get Matt's input on if we want resize updates or no.
// Right now one has to create a host set then add hosts. Is it worth
// notifying for the "no-op" add if there may not be hosts?
runUpdateCallbacks(i, {}, {});
So right now we always runUpdateCallbacks() any time we change the priority set. So if you getOrCreate(P=5) nd add hosts, you'll get updates for 2,3,4,5 [new hosts for 5]
My thought was it's worth noting when a host set is added but if there are no hosts, does it matter? As long as all callback subscribers resize their data structures for 5 when they get the host update I think we can do away with the no-op callbacks.
Happy to make that change as a standalone or as part of the load balancer update.
There was a problem hiding this comment.
I see. IMO I would probably get rid of the NOP default callback unless we have a specific use case for it. This confused me when I read the code (though the explanation makes sense). I was actually worried about it in the context of the initial (0) set creation, and whether premature callbacks would do something bad during init. Again I could go either way on this as long as it is commented well, but my preference would be to go for simplicity.
There was a problem hiding this comment.
SG, will remove it in the follow-up and split it out if it's a larger change than I think it is.
No-op in the interim since no one should be using P!=0 and we don't have callbacks for P=0.
There was a problem hiding this comment.
We don't have callbacks for P=0 because the callbacks aren't installed yet right? (Not because there is a guard in the callback code)?
This change just passes priority sets to the load balancers instead of host sets.
This required implementing priority subsets for the subset LB, which made it a large enough change without actually using the new priorities for any of the load balancers.
The next step for #1929
Risk Level: Medium
This includes removing the default constructor for priority sets to allow sub-classing for the PrioritySubset. Both places where priority sets are used in the code default-create as the LB code still assumes priority 0 exists.
Testing:
Existing tests pass.
One unit test for the new subset lb code not causing harm with priority = 1
More tests will come in the next PR, where we actually use non-zero priority levels.
No release notes for this one, since it's a no-op for functionality. I'll add release notes where I add LB supprt.