[GitHub] [kafka] kkonstantine commented on a change in pull request #8445: KAFKA-9823: Remember the sent generation for the coordinator request
kkonstantine commented on a change in pull request #8445: URL: https://github.com/apache/kafka/pull/8445#discussion_r411125481 ## File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java ## @@ -737,16 +751,21 @@ public void handle(SyncGroupResponse syncResponse, log.debug("SyncGroup failed because the group began another rebalance"); future.raise(error); } else if (error == Errors.FENCED_INSTANCE_ID) { -log.error("Received fatal exception: group.instance.id gets fenced"); +// for sync-group request, even if the generation has changed we would not expect the instance id +// gets fenced, and hence we always treat this as a fatal error +log.error("SyncGroup failed with {} due to group.instance.id {} gets fenced", +sentGeneration, rebalanceConfig.groupInstanceId); future.raise(error); } else if (error == Errors.UNKNOWN_MEMBER_ID || error == Errors.ILLEGAL_GENERATION) { -log.debug("SyncGroup failed: {}", error.message()); -resetGenerationOnResponseError(ApiKeys.SYNC_GROUP, error); +log.info("SyncGroup failed with {}: {}, would request re-join", sentGeneration, error.message()); Review comment: Not sure how this comment was changed, but this still doesn't read well for me. Still seems that the message should say `... will request re-join"` 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
[GitHub] [kafka] kkonstantine commented on a change in pull request #8445: KAFKA-9823: Remember the sent generation for the coordinator request
kkonstantine commented on a change in pull request #8445: URL: https://github.com/apache/kafka/pull/8445#discussion_r411123324 ## File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java ## @@ -737,16 +751,21 @@ public void handle(SyncGroupResponse syncResponse, log.debug("SyncGroup failed because the group began another rebalance"); future.raise(error); } else if (error == Errors.FENCED_INSTANCE_ID) { -log.error("Received fatal exception: group.instance.id gets fenced"); +// for sync-group request, even if the generation has changed we would not expect the instance id +// gets fenced, and hence we always treat this as a fatal error +log.error("SyncGroup failed with {} due to group.instance.id {} gets fenced", +sentGeneration, rebalanceConfig.groupInstanceId); future.raise(error); } else if (error == Errors.UNKNOWN_MEMBER_ID || error == Errors.ILLEGAL_GENERATION) { -log.debug("SyncGroup failed: {}", error.message()); -resetGenerationOnResponseError(ApiKeys.SYNC_GROUP, error); +log.info("SyncGroup failed with {}: {}, would request re-join", sentGeneration, error.message()); +if (generationUnchanged()) Review comment: Makes sense. Thanks 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