[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire

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

dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916166251


##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -1112,9 +1121,10 @@ public synchronized RequestFuture<Void> maybeLeaveGroup(String leaveReason) {
             // attempt any resending if the request fails or times out.
             log.info("Member {} sending LeaveGroup request to coordinator {} due to {}",
                 generation.memberId, coordinator, leaveReason);
+            final String reason = truncateIfRequired(leaveReason);
             LeaveGroupRequest.Builder request = new LeaveGroupRequest.Builder(
                 rebalanceConfig.groupId,
-                Collections.singletonList(new MemberIdentity().setMemberId(generation.memberId).setReason(leaveReason))
+                Collections.singletonList(new MemberIdentity().setMemberId(generation.memberId).setReason(reason))

Review Comment:
   nit: Should we call `truncateIfRequired` inline here? Is it possible to also test this path?



##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -1050,10 +1051,18 @@ public synchronized void requestRejoin(final String shortReason) {
     public synchronized void requestRejoin(final String shortReason,
                                            final String fullReason) {
         log.info("Request joining group due to: {}", fullReason);
-        this.rejoinReason = shortReason;
+        this.rejoinReason = truncateIfRequired(shortReason);

Review Comment:
   In my opinion, it would be better to do this when the JoinGroupRequestData is created. It is here: https://github.com/apache/kafka/blob/64ac302b1c6baa4b28e6fb90776985ac242d41e3/clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java#L563. This ensures that we cover all the paths.



##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java:
##########
@@ -571,6 +571,11 @@ public void testRejoinReason() {
         expectSyncGroup(generation, memberId);
         ensureActiveGroup(generation, memberId);
         assertEquals("", coordinator.rejoinReason());
+
+        // check limit length of reason field
+        mockClient.prepareResponse(joinGroupFollowerResponse(defaultGeneration, memberId, JoinGroupRequest.UNKNOWN_MEMBER_ID, Errors.GROUP_MAX_SIZE_REACHED));

Review Comment:
   It would be better to do a test similar to the one at L557. That one verifies the content put in the JoinGroupRequest as well.



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