[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916314304
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##
@@ -1112,9 +1121,10 @@ public synchronized RequestFuture
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:
makes sense to inline, thanks!
As for the test, though I'm still catching up with the code base, I'll check
if the argument of the `send` method could be asserted.
Does that sound fine?
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916314304
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##
@@ -1112,9 +1121,10 @@ public synchronized RequestFuture
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:
makes sense to inline, thanks!
As for the test, though I'm still catching up with the code base, I'll check
if the `request` argument of the `send` method could be asserted.
https://github.com/apache/kafka/blob/64ac302b1c6baa4b28e6fb90776985ac242d41e3/clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java#L1130
Does that sound fine?
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916622548
##
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:
agree, fixed!
That actually conveys the while change's intention much nicer that the
`reason` is truncated for the sending.
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916623215
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##
@@ -1112,9 +1121,10 @@ public synchronized RequestFuture
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:
thank you!
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916680576
##
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java:
##
@@ -1164,6 +1168,34 @@ public void testHandleNormalLeaveGroupResponse() {
assertTrue(leaveGroupFuture.succeeded());
}
+@Test
+public void testHandleNormalLeaveGroupResponseAndTruncatedLeaveReason() {
+MemberResponse memberResponse = new MemberResponse()
+.setMemberId(memberId)
+.setErrorCode(Errors.NONE.code());
+LeaveGroupResponse leaveGroupResponse =
+leaveGroupResponse(Collections.singletonList(memberResponse));
+String leaveReason = "Very
looong
leaveReason that is 271 characters long to make sure that length limit logic
handles the scenario nicely";
+setupCoordinator(RETRY_BACKOFF_MS, Integer.MAX_VALUE,
Optional.empty());
+
+mockClient.prepareResponse(groupCoordinatorResponse(node,
Errors.NONE));
Review Comment:
thanks, understood about `KafkaAdminClient.java`.
yeah, in my initial attempt I had `setupLeaveGroup(LeaveGroupResponse
leaveGroupResponse, String leaveReason) {`
but I didn't like the assertion bit (as it hides the truncation)
```
LeaveGroupRequestData leaveGroupRequest = ((LeaveGroupRequest) body).data();
return leaveGroupRequest.members().get(0).memberId().equals(memberId) &&
leaveGroupRequest.members().get(0).reason().equals(leaveReason.substring(0,
255));
}, leaveGroupResponse);
```
however, maybe it's not that bad.
alternatively, we can pass `actualLeaveReason` and `expectedLeaveReason` but
it looks even scarier.
I can try the former approach and show you the result.
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916752022
##
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java:
##
@@ -1164,6 +1168,34 @@ public void testHandleNormalLeaveGroupResponse() {
assertTrue(leaveGroupFuture.succeeded());
}
+@Test
+public void testHandleNormalLeaveGroupResponseAndTruncatedLeaveReason() {
+MemberResponse memberResponse = new MemberResponse()
+.setMemberId(memberId)
+.setErrorCode(Errors.NONE.code());
+LeaveGroupResponse leaveGroupResponse =
+leaveGroupResponse(Collections.singletonList(memberResponse));
+String leaveReason = "Very
looong
leaveReason that is 271 characters long to make sure that length limit logic
handles the scenario nicely";
+setupCoordinator(RETRY_BACKOFF_MS, Integer.MAX_VALUE,
Optional.empty());
+
+mockClient.prepareResponse(groupCoordinatorResponse(node,
Errors.NONE));
Review Comment:
@dajac I've drafted the change for the `KafkaAdminClient` (you've mentioned
it earlier, sorry overlooked), it feels that the method `truncateIfRequired`
should be coverted to an util one.
I'm thinking of putting it in `org.apache.kafka.common.utils.Utils` but
could you please give any suggestion if there's a more suitable place?
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r917905764
##
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##
@@ -1432,4 +1432,17 @@ public static String[] enumOptions(Class> enumClass) {
.toArray(String[]::new);
}
+/**
+ * Ensures that the provided {@code reason} remains within a range of 255
chars.
+ * @param reason This is the reason that is sent to the broker over the
wire
+ * as a part of {@code JoinGroupRequest}, {@code
LeaveGroupRequest} or {@code RemoveMembersFromConsumerGroupOptions} messages.
+ * @return a provided reason as is or truncated reason if it exceeds the
255 chars threshold.
+ */
+public static String truncateIfRequired(final String reason) {
Review Comment:
thanks for the suggestions!
yeah `maybeTruncateReason` for sure is a better name, also I've noticed
there are a few method names that start with "maybe" so it will be consistent.
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r917938399
##
clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java:
##
@@ -70,6 +70,21 @@ public static void validateGroupInstanceId(String id) {
});
}
+/**
+ * Ensures that the provided {@code reason} remains within a range of 255
chars.
+ * @param reason This is the reason that is sent to the broker over the
wire
+ * as a part of {@code JoinGroupRequest}, {@code
LeaveGroupRequest}
+ * or {@code RemoveMembersFromConsumerGroupOptions} messages.
Review Comment:
though it looks like a straightforward change, probably I need to spend more
time digesting it.
--
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
