showuon commented on a change in pull request #11566:
URL: https://github.com/apache/kafka/pull/11566#discussion_r770265543
##########
File path:
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java
##########
@@ -486,6 +487,43 @@ public void testRetainMemberIdAfterSyncGroupDisconnect() {
ensureActiveGroup(rejoinedGeneration, memberId);
}
+ @Test
+ public void testRejoinReason() {
+ setupCoordinator();
+
+ String memberId = "memberId";
+ int generation = 5;
+
+ // test initial reason
+ mockClient.prepareResponse(groupCoordinatorResponse(node,
Errors.NONE));
+ expectJoinGroup("", "initialized abstract coordinator", generation,
memberId);
+
+ // successful sync group response should reset reason
+ expectSyncGroup(generation, memberId);
+ ensureActiveGroup(generation, memberId);
+ assertEquals("", coordinator.rejoinReason());
+
+ // Force a rebalance
+ expectJoinGroup(memberId, "Manual test trigger", generation, memberId);
+ expectSyncGroup(generation, memberId);
+ coordinator.requestRejoin("Manual test trigger");
+ ensureActiveGroup(generation, memberId);
+ assertEquals("", coordinator.rejoinReason());
+
+ // max group size reached
+
mockClient.prepareResponse(joinGroupFollowerResponse(defaultGeneration,
memberId, JoinGroupRequest.UNKNOWN_MEMBER_ID, Errors.GROUP_MAX_SIZE_REACHED));
+ coordinator.requestRejoin("Manual test trigger 2");
+ try {
+ coordinator.joinGroupIfNeeded(mockTime.timer(100L));
+ } catch (GroupMaxSizeReachedException e) {
Review comment:
any reason why we don't use `assertThrows` here?
##########
File path:
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java
##########
@@ -559,8 +606,10 @@ private void expectJoinGroup(
return false;
}
JoinGroupRequestData joinGroupRequest = ((JoinGroupRequest)
body).data();
+ boolean isReasonMatching = expectedReason == null ||
joinGroupRequest.reason().equals(expectedReason);
Review comment:
I don't think the logic here is correct.
If the `expectedReason` is null, but the `joinGroupRequest.reason()` is not
null, should it be passed?
That also implies you need add tests to cover this situation.
--
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
