autosharding: implement the sliceMap - #9380
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9380 +/- ##
==========================================
+ Coverage 87.54% 87.56% +0.01%
==========================================
Files 429 430 +1
Lines 30622 30661 +39
==========================================
+ Hits 26807 26847 +40
+ Misses 3814 3813 -1
Partials 1 1
🚀 New features to boost your workflow:
|
| sort.Slice(states, func(i, j int) bool { | ||
| return states[i].index < states[j].index | ||
| }) | ||
| sm.fallbackPool = make([]int, len(states)) |
There was a problem hiding this comment.
Why are we creating a temporary states populating it , sorting and then storing indexes in fallback. Cant we just store the indexes in fallback and then sort ? Or am I missing something ?
| { | ||
| name: "valid-assignment", | ||
| assignment: &assignment{ | ||
| endpointNames: []string{"hostA", "hostB", "hostC", "hostD"}, |
There was a problem hiding this comment.
Just curious : Here "hostD" is also unknown right ??
There was a problem hiding this comment.
Yes, you are right. Removed it.
| } | ||
| } | ||
|
|
||
| sm.slices = append(sm.slices, entry) |
There was a problem hiding this comment.
We need to add one more entry here, example
if assignment are
[1,2, ]
[2,6, ]
[6,8, ]
SliceMap has to be
[-inf , 1) -> Fallback
[1, 2) - >
[2,6) - >
[6, 8) ->
[8, +inf) -> fallback
I you are keeping just start key -inf case can be handled in logic, but you need one more entry for last +inf case where you need to insert an entry in map.
There was a problem hiding this comment.
Let me know if that make sense. Or I am misreading logic here
There was a problem hiding this comment.
The AssignmentProvider would take care of ensuring that there are no gaps in the assignments, so that the sliceMap implementation does not have to worry about it.
I've also expanded the docstring on the assignment type to clarify this a little.
eshitachandwani
left a comment
eshitachandwani
left a comment
There was a problem hiding this comment.
LGTM , with 2 very minor nits.
| return &autoshardingBalancer{} | ||
| } | ||
|
|
||
| // slice represents a key range and its assigned endpoints. |
There was a problem hiding this comment.
nit :
| // slice represents a key range and its assigned endpoints. | |
| // slice represents a key range and indices of its assigned endpoints. |
| endpoints []int // Indices into list[PickerEndpoint] in the Picker | ||
| } | ||
|
|
||
| // sliceMap is a data structure optimized for lookups. Given a key, it returns a |
There was a problem hiding this comment.
nit: Can we remove / reword the second sentence here ? It makes it seem like a function and makes it a little confusing
3 participants
This PR only contains the implementation of the
sliceMap. My plan is to make a few small PRs initially with things that can be separated out, before a bigger PR that contains the rest of the implementation of the LB policy. The implementation of thesliceMapfollows the spec very closely, so should be reviewable even without the rest of the changes.RELEASE NOTES: none