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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
