KAFKA-3014: fix integer overflow problem in leastLoadedNode #696
Conversation
|
Should be noted that overflow is technically still possible in this patch, but the cluster size would have to be greater than Integer.MAX_VALUE/2 to see it. |
| for (int i = 0; i < nodes.size(); i++) { | ||
| int idx = Utils.abs((this.nodeIndexOffset + i) % nodes.size()); | ||
| int idx = (offset + i) % nodes.size(); |
There was a problem hiding this comment.
Could this be completely fixed by
int startIdx = Utils.abs(this.randOffset.nextInt(Integer.MAX_VALUE)) % nodes.size();
...
for (int i = 0; i < nodes.size; i++) {
int idx = (startIdx + i) % nodes.size();
}
There was a problem hiding this comment.
I could be wrong, but that seems equivalent to what I have above. If we're really concerned about this overflow, the easiest thing would be to do the addition using longs and convert back to int after doing the modulus. Seems like overkill though since no one is likely to scale Kafka past a billion nodes. And the impact of overflow is benign anyway when you have so many nodes to choose from.
There was a problem hiding this comment.
You are right, they are actually the same.
Author: Jason Gustafson <jason@confluent.io> Reviewers: Guozhang Wang Closes #696 from hachikuji/KAFKA-3014 (cherry picked from commit fa4ffb8) Signed-off-by: Guozhang Wang <wangguoz@gmail.com>
|
LGTM. Merged to both branches. |
No description provided.