frankvicky commented on code in PR #17614:
URL: https://github.com/apache/kafka/pull/17614#discussion_r1981011690
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -427,7 +428,7 @@ public void onGroupAssignmentUpdated(Set<TopicPartition>
partitions) {
// call close methods if internal objects are already constructed;
this is to prevent resource leak. see KAFKA-2121
// we do not need to call `close` at all when `log` is null, which
means no internal objects were initialized.
if (this.log != null) {
- close(Duration.ZERO, true);
+ close(Duration.ZERO,
CloseOptions.GroupMembershipOperation.DEFAULT, true);
Review Comment:
I just looked at `GroupMetadataManager` and found a point that needs
discussion:
The KIP has already defined how to handle leave behavior, where classic
consumers refer to whether or not to send a request. What's more questionable
is for async consumers.
The current implementation is:
- For `LEAVE_GROUP`: Both static and dynamic carry epoch `-1`
- For `REMAIN_IN_GROUP`: Both static and dynamic carry epoch `-2`
- For `DEFAULT`: dynamic carries epoch `-1`, static carries epoch `-2`
The problem is, in the current `GroupMetadataManager` code, as long as it
finds that the epoch is `-1` or `-2`, it enters the leave process, then further
distinguishes between dynamic or static.
First, the uncontroversial part: If it's static (`instanceId != null`), `-2`
will retain membership until session timeout. Conversely, `-1` means exit.
https://github.com/apache/kafka/blob/1bfa4cd17be3ad3dd6e8b97dd0a2c9f2d43c89aa/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java#L3166-L3177
The controversial part: If it's dynamic, `-1` means exit. If it's `-2`, it
will throw an `InvalidRequestException` due to an invalid request before
entering the leave process.
https://github.com/apache/kafka/blob/1bfa4cd17be3ad3dd6e8b97dd0a2c9f2d43c89aa/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java#L1419-L1420
If we agree to change the definition of special epochs (`-1` for leaving the
group, `-2` for remaining in the group), I think we should modify the
`throwIfConsumerGroupHeartbeatRequestIsInvalid` check and follow the pattern of
static members(e.g., `consumerGroupStaticMemberGroupLeave`)
c.c @lianetm @chia7712
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
