chickenchickenlove commented on code in PR #21565:
URL: https://github.com/apache/kafka/pull/21565#discussion_r2871579612
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -8733,6 +8986,18 @@ private Map<String, String>
streamsGroupAssignmentConfigs(String groupId) {
return Map.of("num.standby.replicas", numStandbyReplicas.toString());
}
+ static boolean hasEpochRelevantMemberConfigChanged(
+ StreamsGroupMember oldMember,
+ StreamsGroupMember newMember
+ ) {
+ // The group epoch is bumped: (KIP-1071)
+ // - When a member updates its topology metadata, rack ID, client tags
or process ID.
+ return !Objects.equals(oldMember.topologyEpoch(),
newMember.topologyEpoch())
+ || !Objects.equals(oldMember.rackId(), newMember.rackId())
Review Comment:
@lucasbru @squah-confluent
I’d also like to discuss this.
In KIP-1071, it explicitly states that the group epoch should be bumped only
in those specific cases.
```
The group epoch is bumped:
- When a member joins or leaves the group.
- When a member is fenced or removed from the group by the group coordinator.
- When the partition metadata is updated. For instance when a new partition
is added or a new topic matching the subscribed topics is created.
- When a member with an assigned warm-up task reports a task changelog
offset and task changelog end offset whose difference is less than
acceptable.recovery.lag.
- When a member updates its topology metadata, rack ID, client tags or
process ID. Note: Typically, these do not change within the lifetime of a
Streams client, so this only happens when a member with static membership
rejoins with an updated configuration.
- When an assignment configuration for the group is updated.
```
However, the current code seems to evaluate the group-epoch bump condition
more strictly than what KIP-1071 describes.
On the other hand, KIP-1071 also includes the following sentence:
```
Updates information of the member if needed. The group epoch is incremented
if there is any change.
```
So there appears to be a conflict between the explicitly listed conditions
for bumping the group epoch and the statement that the group epoch is
incremented whenever anything changes. I think we should resolve this
inconsistency. Once it’s clarified, we may need to adjust this method and the
call sites that depend on it.
What do you think?
If I’m misunderstanding anything, please let me know! 🙇♂️
--
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]
