KAFKA-20778: Avoid unneeded rebalance on classic consumer when replica unavailable#22768
Merged
Merged
Conversation
dajac
reviewed
Jul 7, 2026
| * a rack that is actually added or removed still is. | ||
| */ | ||
| boolean equivalentTo(PartitionRackInfo other) { | ||
| return racksAccountedForIn(other) && other.racksAccountedForIn(this); |
Member
There was a problem hiding this comment.
Do we really need to check both ways here?
Member
Author
There was a problem hiding this comment.
the first one is for catching if a rack was removed (not related to an unvailable replica), needs rebalance, but does not catch if one was added (needs rebalance too), that's why checking both ways. Makes sense?
| // Ids of the replicas whose broker is not in the current live-broker list (they resolve | ||
| // to an empty node), so their rack is unknown. Tracked by id so that a broker that is | ||
| // temporarily unavailable is not mistaken for a rack change. | ||
| private final Set<Integer> unknownReplicas; |
Member
There was a problem hiding this comment.
nit: I wonder whether online and offline would be a better terminology. Not sure...
Member
Author
There was a problem hiding this comment.
yes, I think it's better too, done
| // (replica 1 remains in the partition replicas, with an unknown rack) and then comes | ||
| // back on the same replica. Neither transition is a rack change, so no rebalance is | ||
| // expected at any point. | ||
| List<String> racks = Arrays.asList("rack-a", "rack-b", "rack-c"); |
Member
There was a problem hiding this comment.
nit: Should we use List.of and Map.of where possible?
Member
Author
|
Thanks @dajac ! Comments addressed |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Improve to avoid unnecessary double rebalance on the classic consumer
when a replica is bounced.
Before this PR, when a replica is bounced it resolves to an empty node
(no host) with a null rack on the consumer metadta view. The consumer's
metadata comparison treated that as a rack change (rack1 to null) and
triggered a rebalance, then a second one when the broker came back (null
to rack1). This affects the classic consumer, every assignor, when
client.rack is configured.
This PR improves the rack comparison to track racks by replica id and
treat an unavailable replica (empty node) as having an unknown rack
rather than a changed one: two metadata snapshots are equivalent if they
expose the same set of racks for available replicas, tolerating a
missing/unknown rack only when the replica it belongs to is unavailable.
Reviewers: David Jacot david.jacot@gmail.com