ableegoldman commented on code in PR #17614:
URL: https://github.com/apache/kafka/pull/17614#discussion_r2029660460


##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerMembershipManager.java:
##########
@@ -469,8 +493,17 @@ public int joinGroupEpoch() {
      */
     @Override
     public int leaveGroupEpoch() {
-        return groupInstanceId.isPresent() ?
-                ConsumerGroupHeartbeatRequest.LEAVE_GROUP_STATIC_MEMBER_EPOCH :
-                ConsumerGroupHeartbeatRequest.LEAVE_GROUP_MEMBER_EPOCH;
+        boolean isStaticMember = groupInstanceId.isPresent();
+        if (REMAIN_IN_GROUP == leaveGroupOperation && isStaticMember) {
+            return 
ConsumerGroupHeartbeatRequest.LEAVE_GROUP_STATIC_MEMBER_EPOCH;
+        }
+
+        if (LEAVE_GROUP == leaveGroupOperation) {
+            return ConsumerGroupHeartbeatRequest.LEAVE_GROUP_MEMBER_EPOCH;

Review Comment:
   I think I understand why this condition is needed but it would be good to 
leave a comment explaining this change because it's pretty obscure. IIUC 
basically the problem is that there's no way to permanently leave the group 
with a static consumer right now, and the only method of removing static 
members is via an admin API. So we essentially have to "trick" the 
GroupMetadataManager into fencing this member to kick it out of the group, 
because if we use the `LEAVE_GROUP_STATIC_MEMBER_EPOCH ` then it thinks it's 
just being "temporarily" removed and this won't actually result in the consumer 
leaving the group. So we use `LEAVE_GROUP_MEMBER_EPOCH` because anything that 
isn't the `LEAVE_GROUP_STATIC_MEMBER_EPOCH` triggers the GroupMetadataManager 
to fence the consumer in its `#consumerGroupLeave` method.
   
   First off does that summary sound right? It's also pretty hacky, and while I 
think it's fine to proceed with this for now, I'm wondering if we shouldn't 
just try to implement proper LeaveGroup mechanics for static membership. I know 
this would require some server-side changes but maybe this can be a followup 
KIP? Again, no need to block this PR on that, just putting it out there. Feel 
free to pick that up yourself or file a ticket if that makes sense to you too



-- 
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

Reply via email to