lianetm commented on code in PR #16449:
URL: https://github.com/apache/kafka/pull/16449#discussion_r1665774645
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java:
##########
@@ -2121,6 +2136,8 @@ private void
assertStaleMemberLeavesGroupAndClearsAssignment(MembershipManagerIm
assertTrue(membershipManager.currentAssignment().isNone());
assertTrue(membershipManager.topicsAwaitingReconciliation().isEmpty());
assertEquals(LEAVE_GROUP_MEMBER_EPOCH,
membershipManager.memberEpoch());
+ membershipManager.leaveGroup();
+ verify(subscriptionState).unsubscribe();
Review Comment:
uhm I would say we don't want to force a leave group here, because it
changes the semantics of all the tests around stale members. All stale members
send a leave group request to the broker (therefore the test name), but not all
stale members receive an API-triggered call to leaveGroup (diff stuff). So we
have 2 scenarios for stale members here:
1. member becomes stale + consumer poll -> it will rejoin the group reusing
the same subscription it had. This is probably the common case, working fine
before this PR (stale clears assignment only when it completes callbacks
[here](https://github.com/apache/kafka/blob/cb63fdb77ed869f2c519ca002ca757425b6b3076/clients/src/main/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImpl.java#L900))
1. member goes STALE + consumer leaves group before the stale member has
the chance to rejoin -> we want to clear the subscription, because of the
"external" leave group call. This is the edge case this PR fixes and adds tests
for, you already added `testLeaveGroupWhenStateIsStale`.
Actually, for the common case 1 of stale members that will rejoin (do not
receive a forced call to leaveGroup), we should `verify(subscriptionState,
never()).unsubscribe();`.We should add that line here instead. Makes sense?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
