guozhangwang commented on code in PR #13190:
URL: https://github.com/apache/kafka/pull/13190#discussion_r1119357976
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -500,14 +500,22 @@ boolean joinGroupIfNeeded(final Timer timer) {
requestRejoin(shortReason, fullReason);
}
+ // continue to retry as long as the timer hasn't expired
Review Comment:
Could we simplify this multi-if logic as:
```
if (!future.isRetriable()) { throw }
else {
if (timer.isExpired() { return false }
else if (exception instance of.. ) { continue}
else {timer.sleep(..)}
}
```
Also could we add a comment on top clarifying that the order of precedence
are deliberated in this order and future changes should pay attention to not
change it unnecessarily.
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinatorTest.java:
##########
@@ -1484,6 +1484,8 @@ public void testRebalanceWithMetadataChange() {
Utils.mkMap(Utils.mkEntry(topic1, 1), Utils.mkEntry(topic2,
1))));
client.respond(joinGroupFollowerResponse(1, consumerId, "leader",
Errors.NOT_COORDINATOR));
client.prepareResponse(groupCoordinatorResponse(node, Errors.NONE));
+ coordinator.poll(time.timer(0)); // failing joinGroup request will
require re-poll in order to retry
Review Comment:
It's not very clear to me why here and line 3403 below we need additional
polls since the test scenarios seems irrelevant to error cases?
--
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
