loadbalancer-experimental: add support for weights in round robin #2909
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryce-anderson
requested review from
daschl,
mgodave,
tkountis and
idelpivnitskiy
and removed request for
daschl and
idelpivnitskiy
tkountis
approved these changes
May 7, 2024
...-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinSelector.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| int nextHost() { | ||
| return (int) (Integer.toUnsignedLong(index.getAndIncrement()) % hostsSize); |
There was a problem hiding this comment.
- Consider keeping original
(index.getAndIncrement() & Integer.MAX_VALUE)to avoid a cast fromlong. - Pre-existing, but if we are here wanna ask: when
indexoverflows we loose the natural index increment. For example, if there are 13 hosts it will jump from idx=10 to idx=0 on overflow instead of going to inx=11. Should we worry about it or is it considered "ok" to interrupt a real round-robin once in a while?
There was a problem hiding this comment.
- I like the
.toUnsignedLongbecause it doubles the range an int can represent. The cast is clearly safe sincehostSizeis bounded to int sizes. - I'm not worried about a jump every ~4 billion requests.
There was a problem hiding this comment.
- No worried about safety of the cast. If we see a trivial way to avoid unnecessary operation on the hot path, every small effort counts. I wouldn't say it's "premature optimization" bcz this is what we already had.
- Ok.
| @Override | ||
| int nextHost() { | ||
| while (true) { | ||
| long counter = Integer.toUnsignedLong(index.getAndIncrement()); |
There was a problem hiding this comment.
I would propose to use the same trick here to avoid long completely
There was a problem hiding this comment.
Same as above: I think it's mostly subjective but I like the expanded range without switching to an AtomicLong.
There was a problem hiding this comment.
My expectation is that we can stay in int without even going to long or AtomicLong
...-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinSelector.java
Show resolved
Hide resolved
...-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/RoundRobinSelector.java
Outdated
Show resolved
Hide resolved
idelpivnitskiy
approved these changes
May 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
We want to support weighted but don't in the round-robin
HostSelector.
Modifications:
Add weight support to round-robin using the static stride
algorithm common to the grpc libraries.