[GitHub] [kafka] dajac commented on a change in pull request #11571: KAFKA-13496: add reason to LeaveGroupRequest

[GitBox <gi...@apache.org>]

dajac commented on a change in pull request #11571:
URL: https://github.com/apache/kafka/pull/11571#discussion_r769741518



##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -3731,6 +3731,11 @@ public RemoveMembersFromConsumerGroupResult removeMembersFromConsumerGroup(Strin
         } else {
             members = options.members().stream().map(MemberToRemove::toMemberIdentity).collect(Collectors.toList());
         }
+
+        if (options.reason() != null) {
+            members.forEach(member -> member.setReason(options.reason()));

Review comment:
       I think the KIP also specifies a default reason in this case, doesn't it?

##########
File path: core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala
##########
@@ -647,7 +647,7 @@ class GroupCoordinator(val brokerId: Int,
                   if (memberId == JoinGroupRequest.UNKNOWN_MEMBER_ID) {
                     groupInstanceId.flatMap(group.currentStaticMemberId) match {
                       case Some(currentMemberId) =>
-                        removeCurrentMemberFromGroup(group, currentMemberId)
+                        removeCurrentMemberFromGroup(group, currentMemberId, leavingMember.reason())

Review comment:
       nit: Parenthesis could be omitted.

##########
File path: clients/src/main/resources/common/message/LeaveGroupRequest.json
##########
@@ -36,7 +38,10 @@
         "about": "The member ID to remove from the group." },
       { "name": "GroupInstanceId", "type": "string",
         "versions": "3+", "nullableVersions": "3+", "default": "null",
-        "about": "The group instance ID to remove from the group." }
+        "about": "The group instance ID to remove from the group." },

Review comment:
       Could we extend tests in `RequestResponseTest` to cover this new field?

##########
File path: core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala
##########
@@ -617,9 +617,9 @@ class GroupCoordinator(val brokerId: Int,
                        leavingMembers: List[MemberIdentity],
                        responseCallback: LeaveGroupResult => Unit): Unit = {
 
-    def removeCurrentMemberFromGroup(group: GroupMetadata, memberId: String): Unit = {
+    def removeCurrentMemberFromGroup(group: GroupMetadata, memberId: String, leaveReason: String): Unit = {

Review comment:
       Should we use `Option[String]` for the `leaveReason` and handle the case where it is not provided?

##########
File path: core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala
##########
@@ -665,7 +665,7 @@ class GroupCoordinator(val brokerId: Int,
                       groupInstanceId,
                       operation = "leave-group"
                     ).getOrElse {
-                      removeCurrentMemberFromGroup(group, memberId)
+                      removeCurrentMemberFromGroup(group, memberId, leavingMember.reason())

Review comment:
       ditto.




-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org