You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/07/07 07:23:23 UTC
[GitHub] [kafka] etolbakov opened a new pull request, #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
etolbakov opened a new pull request, #12388:
URL: https://github.com/apache/kafka/pull/12388
Hello David @dajac ,
I found an open JIRA ticket - https://issues.apache.org/jira/browse/KAFKA-14013
and decided to come up with a suggestion.
Could you please take a look if you have a spare minute?
Happy to adjust the changes if that's required
--
Regards, Eugene
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916162985
##########
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.
--
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
[GitHub] [kafka] jnh5y commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
jnh5y commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1178280579
> > @etolbakov Thanks for the PR. When the jira is assigned, it usually means that someone is working on it. In this case, I know that @jnh5y wanted to fix this. @jnh5y If you haven't started yet, we could perhaps review this one.
>
> Thank you @dajac for finding time for a review! Sorry for creating confusion, indeed I should have asked you or @jnh5y first (was too excited about the change 😅). Noted for the future. I will address your suggestions ASAP.
No worries, it is all yours! I could have assigned the ticket to myself, etc.
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r917780526
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -1432,4 +1432,17 @@ public static String[] enumOptions(Class<? extends Enum<?>> 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:
nit: As this is tight to the reason, I would rather put it in `JoinGroupRequest`. Should we call it `maybeTruncateReason`?
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -4084,6 +4084,13 @@ public void testRemoveMembersFromGroupReason() throws Exception {
testRemoveMembersFromGroup("testing remove members reason", "testing remove members reason");
}
+ @Test
+ public void testRemoveMembersFromGroupReasonAndTruncateReason() throws Exception {
Review Comment:
nit: `testRemoveMembersFromGroupTruncatesReason`?
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -478,11 +478,12 @@ boolean joinGroupIfNeeded(final Timer timer) {
resetJoinGroupFuture();
synchronized (AbstractCoordinator.this) {
+ final String simpleName = exception.getClass().getSimpleName();
final String shortReason = String.format("rebalance failed due to %s",
- exception.getClass().getSimpleName());
+ simpleName);
Review Comment:
nit: Could we put this one on the previous line?
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java:
##########
@@ -1193,6 +1215,12 @@ public void testHandleLeaveGroupResponseWithException() {
}
private RequestFuture<Void> setupLeaveGroup(LeaveGroupResponse leaveGroupResponse) {
+ return setupLeaveGroup(leaveGroupResponse, "test maybe leave group", "test maybe leave group");
+ }
+
+ private RequestFuture<Void> setupLeaveGroup(LeaveGroupResponse leaveGroupResponse,
+ String expectedLeaveReason,
+ String actualLeaveReason) {
Review Comment:
nit: I would put `actualLeaveReason` first. It is a bit more natural. Should we call it `leaveReason`?
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -478,11 +478,12 @@ boolean joinGroupIfNeeded(final Timer timer) {
resetJoinGroupFuture();
synchronized (AbstractCoordinator.this) {
+ final String simpleName = exception.getClass().getSimpleName();
final String shortReason = String.format("rebalance failed due to %s",
- exception.getClass().getSimpleName());
+ simpleName);
final String fullReason = String.format("rebalance failed due to '%s' (%s)",
- exception.getMessage(),
- exception.getClass().getSimpleName());
+ exception.getMessage(),
+ simpleName);
Review Comment:
nit: Could we revert to the previous indentation?
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinatorTest.java:
##########
@@ -1204,11 +1232,11 @@ private RequestFuture<Void> setupLeaveGroup(LeaveGroupResponse leaveGroupRespons
}
LeaveGroupRequestData leaveGroupRequest = ((LeaveGroupRequest) body).data();
return leaveGroupRequest.members().get(0).memberId().equals(memberId) &&
- leaveGroupRequest.members().get(0).reason().equals("test maybe leave group");
+ leaveGroupRequest.members().get(0).reason().equals(expectedLeaveReason);
Review Comment:
nit: Could we remove that extra added space?
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
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<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:
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac merged PR #12388:
URL: https://github.com/apache/kafka/pull/12388
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916672037
##########
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 looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong 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:
Instead of duplicating all this code, could we reuse `setupLeaveGroup`? We could perhaps adds an overload - e.g. `setupLeaveGroup(leaveGroupResponse, reason)`. What do you think?
--
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
[GitHub] [kafka] etolbakov commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
etolbakov commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1178254261
> @etolbakov Thanks for the PR. When the jira is assigned, it usually means that someone is working on it. In this case, I know that @jnh5y wanted to fix this. @jnh5y If you haven't started yet, we could perhaps review this one.
Thank you @dajac for finding time for a review!
Sorry for creating confusion, indeed I should have asked you or @jnh5y first (was too excited about the change 😅). Noted for the future.
I will address your suggestions ASAP.
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916683217
##########
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 looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong 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:
Using `leaveReason` and `expectedLeaveReason` is OK for me as long as we keep `setupLeaveGroup(leaveGroupResponse)`.
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
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<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:
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1182948012
@etolbakov I don't have any that I can recommend at the moment. You can take a look in Jira though.
--
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
[GitHub] [kafka] etolbakov commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
etolbakov commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1181458638
@dajac David, thanks again for your review!
I'd be very grateful if you could point me to any other (unassigned 😅) ticket that I can take a look at and help with?
--
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
[GitHub] [kafka] dajac commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1178035300
@etolbakov Thanks for the PR. When the jira is assigned, it usually means that someone is working on it. In this case, I know that @jnh5y wanted to fix this. @jnh5y If you haven't started yet, we could perhaps review this one.
--
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
[GitHub] [kafka] etolbakov commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
etolbakov commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1180388125
Thank you for the feedback @dajac!
sorry for the indentation issues, will re-read the contributor recommendations on that matter and make sure it won't be the case again.
--
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
[GitHub] [kafka] dajac commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1180420597
@etolbakov Could you assign https://issues.apache.org/jira/browse/KAFKA-14013 to yourself? If you don't have Jira setup, could you give me your userid and I will set it up for 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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 looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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 looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
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<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:
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] etolbakov commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
etolbakov commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1178751044
@dajac thank you very much for the feedback!
I've made improvements per your suggestions.
Could you please take a look when you have time?
--
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
[GitHub] [kafka] etolbakov commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
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<? extends Enum<?>> 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r917912085
##########
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:
nit: We can remove this line because, in the end, we also send a LeaveGroupRequest in this case.
--
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
[GitHub] [kafka] etolbakov commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
etolbakov commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1180434338
@dajac great news!
Thank you very much for your help & review, David!
my Jira handle `etolbakov`, probably need some permissions to be able to assign tickets to myself.
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12388:
URL: https://github.com/apache/kafka/pull/12388#discussion_r916573678
##########
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:
You can actually do the following in your test:
```
// force a rebalance
expectJoinGroup(memberId, "Manual test trigger", generation, memberId);
expectSyncGroup(generation, memberId);
coordinator.requestRejoin("Manual test trigger");
ensureActiveGroup(generation, memberId);
assertEquals("", coordinator.rejoinReason());
```
`expectJoinGroup` verifies the request sent out.
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by 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
[GitHub] [kafka] dajac commented on pull request #12388: KAFKA-14013: Limit the length of the `reason` field sent on the wire
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12388:
URL: https://github.com/apache/kafka/pull/12388#issuecomment-1181429377
Merged to trunk, 3.3 and 3.2.
--
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