abbccdda commented on a change in pull request #8986:
URL: https://github.com/apache/kafka/pull/8986#discussion_r452997443
##########
File path:
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -460,15 +463,21 @@ boolean joinGroupIfNeeded(final Timer timer) {
exception instanceof IllegalGenerationException ||
exception instanceof MemberIdRequiredException)
continue;
- else if (!future.isRetriable())
- throw exception;
-
- timer.sleep(rebalanceConfig.retryBackoffMs);
+ else {
+ handleFailure(future, timer);
+ }
}
}
return true;
}
+ protected void handleFailure(RequestFuture<?> future, Timer timer) {
+ if (future.isRetriable() || future.exception() instanceof
AuthorizationException)
Review comment:
Yes, I think the current PR scope is fine for just fixing consumer
issue. We should just be careful not to accidentally make retries for other
components relying on subclass of `AuthorizationException`. Could we reorder
the check though, to make throw behavior happen first?
##########
File path:
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java
##########
@@ -259,6 +260,22 @@ public void testCoordinatorDiscoveryBackoff() {
assertTrue(endTime - initialTime >= RETRY_BACKOFF_MS);
}
+ @Test
+ public void testGroupAuthorizationFailure() {
Review comment:
Could we also test topic authorization failure?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
