guozhangwang commented on code in PR #13190:
URL: https://github.com/apache/kafka/pull/13190#discussion_r1120460193
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -500,14 +500,24 @@ boolean joinGroupIfNeeded(final Timer timer) {
requestRejoin(shortReason, fullReason);
}
+ // 4 special non-retriable exceptions that we want to retry,
as long as the timer hasn't expired.
Review Comment:
I think here the comment is not to just state what the code did, since
readers can just understand that from the code :P instead what we want to
emphasize is to remind future contributors that they should be careful to not
change the precedence ordering of this logic unnecessarily.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -500,14 +500,24 @@ boolean joinGroupIfNeeded(final Timer timer) {
requestRejoin(shortReason, fullReason);
}
+ // 4 special non-retriable exceptions that we want to retry,
as long as the timer hasn't expired.
if (exception instanceof UnknownMemberIdException ||
- exception instanceof IllegalGenerationException ||
- exception instanceof RebalanceInProgressException ||
- exception instanceof MemberIdRequiredException)
+ exception instanceof IllegalGenerationException ||
Review Comment:
Ah thanks for the clarifications!
Thinking about this a bit more (sorry for getting back and forth..), I now
concerned a bit more that for some usage patterns where `poll` call would be
triggered less frequently, we may not be coming back to handle these four
exceptions while at the same time the broker is ticking and waiting for the
join-group request to be re-sent. Hence I'm changing my mind to lean a bit more
to honor the exception types for immediate handling than the timeouts ---
again, sorry for going back and forth...
So I think we would define the ordering as the following:
1. For un-retriable exception, always try to handle immediately and not
honor the timer.
2. Otherwise, honor the timer.
In that case, we could just go back to the first time you made the change,
i.e. just add the
```
if (timer.isExpired())
return false;
```
After the `if/else-if` block. Still it's better to comment that above
ordering is diligently designed as such.
--
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
