mjsax commented on PR #23088:
URL: https://github.com/apache/kafka/pull/23088#issuecomment-5218858862
Another round from Claude. The first one is subtle, and I am frankly not
sure about it -- if the logic is not too complex, and we add a proper comment
why we need to treat `num.standby.replicas` differently (ie, 4.3/4.2 downgrade
compatibility), it might indeed to worth fixing it.
```
I flagged the consequence last time but didn't spell out the mechanism, so
here it is. Streams groups are GA since 4.2 (StreamsVersion.SV_1,
LATEST_PRODUCTION, IBP_4_2_IV1), so a 4.3 → 4.4 rolling upgrade with live
streams groups is ordinary. 4.2 and 4.3 write {num.standby.replicas: N}; trunk
today writes byte-identically for any group not using tags, because of the
guard she removed. After this PR the new broker writes {num.standby.replicas:
"0", rack.aware.assignment.tags: ""}. replay reads the key/value list
generically (GroupMetadataManager.java:6236), so when a group's coordinator
moves to a not-yet-upgraded broker, that broker loads tags: "", computes
{nsr:"0"}, sees a difference, and bumps the epoch. The return trip is absorbed
by withoutDefaults, so it's one spurious rebalance per group per new→old
coordinator move during the rolling window — where trunk today has zero. That's
the exact failure mode the JIRA exists to prevent, reintroduced in the
mixed-version window
.
The rule that avoids it is a bit subtle but worth writing down, because
"omit everything at default" is also wrong — omitting num.standby.replicas
would make the old broker see {} vs its {nsr:"0"} and bump for the opposite
reason. What actually keeps things clean is: the written map should stay
byte-identical to what the previous release writes for a group that sets
nothing. Concretely — keep num.standby.replicas unconditional because 4.2/4.3
already write it, and omit newly added configs at their default.
withoutDefaults stays as the general mechanism absorbing legacy maps, and the
guard test still protects the unconditional style for anyone who writes one.
Fair counter-argument for her: it's one bump, transient, and uniformity is
genuinely easier to reason about. Worth raising as a question rather than a
blocker — and Lucas may have a view on how much mixed-version rebalance churn
is acceptable here.
Smaller, new in this revision: the GroupSpec.configs() javadoc now promises
implementers that an absent config is at its default — but the reference
implementation doesn't honor it. StickyTaskAssignor.java:98-100 still does
configs().isEmpty() ? 0 :
Integer.parseInt(configs().get("num.standby.replicas")), so a non-empty map
missing that key throws NumberFormatException instead of defaulting.
Unreachable today, but it's a public contract statement for KIP-1357 assignors
that the built-in one breaks; getOrDefault(..., "0") is the whole fix.
Minor: the tags value now reaches assignors as "" rather than being absent —
whoever wires it up will find "".split(",") yields [""], one empty tag, which
the old omit-when-empty shape avoided. And still: withoutDefaults returns a
TreeMap where ordering buys nothing, and heartbeat(...) sits between two @Test
methods.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
