Allow duplicate weighted backend keys #3319
Conversation
sfleen
force-pushed
the
disambig
branch
2 times, most recently
from
501ca8b
to
c58b4af
Compare
|
|
||
| #[derive(Debug)] | ||
| pub struct UnweightedKeys<K> { | ||
| ids: Vec<KeyId>, |
There was a problem hiding this comment.
This is the key part of this change. It decouples the backend keys from the iteration order of the backends with a separate key type. I considered using something like SlotMap, but since we don't modify or remove anything from these maps after they're created we don't need the guarantee of stable keys that it provides and the slight performance cost that comes with it.
|
|
||
| impl<K> UnweightedKeys<K> { | ||
| pub(crate) fn new(iter: impl Iterator<Item = K>) -> Self { | ||
| let mut ids = Vec::new(); |
There was a problem hiding this comment.
This is how the invariant of the IDs and keys being a 1:1 relation is guaranteed by only allowing construction via enumeration of the iterator over keys.
linkerd/distribute/src/params.rs
Outdated
| } | ||
|
|
||
| pub(crate) fn keys(&self) -> &[K] { | ||
| pub(crate) fn make_svc_from_keys<T>( |
There was a problem hiding this comment.
This was another key change. Instead of exposing a slice of keys to create the backend services, this inverts the control to something similar to a combinator to decouple how they keys are iterated over and mapped into services.
There was a problem hiding this comment.
i have a commit here that i'd like to offer for consideration: we only use this to construct our Distribute<K>. we can cut down on non-local reasoning & keep our parameter object a little slimmer by defining this as a private associated function over yonder!
linkerd/distribute/src/stack.rs
Outdated
| .map(|k| (k.clone(), newk.new_service(k.clone()))) | ||
| .collect(); | ||
| Distribute::new(backends, dist) | ||
| Distribute::new(dist, |k| newk.new_service(k.clone())) |
There was a problem hiding this comment.
And now this is much simpler and is decoupled from how the service keys are stored.
There was a problem hiding this comment.
i like the changes in this file! it's nice to not have to do this iter()...collect() dance here at this call-site.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3319 +/- ##
==========================================
- Coverage 67.68% 67.57% -0.11%
==========================================
Files 332 380 +48
Lines 15158 17599 +2441
==========================================
+ Hits 10259 11892 +1633
- Misses 4899 5707 +808
... and 78 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
here's a group of comments, most of which are nits, and one possible bug?
zooming out, i'm curious to tug at whether we can dodge the need to maintain a separate "keyspace" in order to account for duplicate keys.
i'll offer some more thoughts on that in a follow-up comment, but wanted to offer these comments in the meantime! (update 11/05: we paired on this and addressed these thoughts)
linkerd/distribute/src/stack.rs
Outdated
| .map(|k| (k.clone(), newk.new_service(k.clone()))) | ||
| .collect(); | ||
| Distribute::new(backends, dist) | ||
| Distribute::new(dist, |k| newk.new_service(k.clone())) |
There was a problem hiding this comment.
i like the changes in this file! it's nice to not have to do this iter()...collect() dance here at this call-site.
|
I've added an extra refactoring commit that creates separate structs for each distribution variant, which I think makes it a lot clearer and easier to understand. It's a bit more involved, so I'm more than happy to make it a follow-up PR instead if it's too complex for this one. |
Currently, if the proxy receives two backends with the same metadata, one of the backends will get dropped because the backend metadata is used as the key in a hash map. Attempting to then randomly distribute requests to the backends can panic when selecting the now non-existent backend. This is fixed by no longer using backend metadata as a hash map key, instead generating separate IDs that are stored in a vec to retain the declared order of backends while also being used to look up the backend and associated weight independetly. Validated with new unit tests excercising duplicate backend keys, as well as a few around the invariants use to store the backends. Signed-off-by: Scott Fleener <[email protected]>
The existing distribution didn't have a great separation between the different kinds, like having some fields that only apply to one of the variants and not the others. This splits out the distributions into separate structs to make the logic more clear. The structs have been moved to separate files, which makes it much easier to grok the logic for a specific distribution variant. Signed-off-by: Scott Fleener <[email protected]>
Currently, if the proxy receives two backends with the same metadata, one of the backends will get dropped because the backend metadata is used as the key in a hash map. Attempting to then randomly distribute requests to the backends can panic when selecting the now non-existent backend.
This is fixed by no longer using backend metadata as a hash map key, instead generating separate IDs that are stored in a vec to retain the declared order of backends while also being used to look up the backend and associated weight independetly.
Validated with new unit tests excercising duplicate backend keys, as well as a few around the invariants use to store the backends.