squah-confluent commented on code in PR #22510:
URL: https://github.com/apache/kafka/pull/22510#discussion_r3397532402
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -3900,24 +3901,23 @@ private UpdateTargetAssignmentResult<Assignment>
maybeUpdateTargetAssignment(
updatedMember
).orElse(defaultConsumerGroupAssignor.name());
try {
+ UpdatedMembersAndTargetAssignmentView<ConsumerGroupMember,
Assignment> updatedMembersAndTargetAssignment =
+ new UpdatedMembersAndTargetAssignmentView<>(
+ group.members(),
+ group.staticMembers(),
+ group.targetAssignment()
+ );
+
updatedMembersAndTargetAssignment.addOrUpdateMember(updatedMember.memberId(),
updatedMember.instanceId(), updatedMember);
+
TargetAssignmentBuilder.ConsumerTargetAssignmentBuilder
assignmentResultBuilder =
new
TargetAssignmentBuilder.ConsumerTargetAssignmentBuilder(group.groupId(),
groupEpoch, consumerGroupAssignors.get(preferredServerAssignor))
.withTime(time)
- .withMembers(group.members())
- .withStaticMembers(group.staticMembers())
+ .withMembers(updatedMembersAndTargetAssignment.members())
.withSubscriptionType(subscriptionType)
- .withTargetAssignment(group.targetAssignment())
+
.withTargetAssignment(updatedMembersAndTargetAssignment.targetAssignment())
Review Comment:
We don't have a test for that. There are tests for static member replacement
with subscription and assignment changes but no test for subscription change
with no assignment change. Before this PR we did indeed emit a redundant target
assignment record.
I also checked if we could have a bug where we _don't_ write a target
assignment record if the new assignment is empty and there isn't one. We always
write a target assignment record when the previous target assignment is `null`.
How strongly do you feel about adding a test for two target assignment
records vs one? I prefer not to since there's not much impact.
--
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]
