-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-6005: Reject JoinGroup request from first member with empty protocol type/protocol list #3957
Conversation
4fc87ff
to
a719a9d
Compare
@hachikuji if the first group member joins with empty partition.assignment.strategy, then the group won't allow any other members with valid protocols. |
@hachikuji pinging for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch. Left a couple comments.
@@ -146,6 +146,9 @@ class GroupCoordinator(val brokerId: Int, | |||
if (!group.is(Empty) && (!group.protocolType.contains(protocolType) || !group.supportsProtocols(protocols.map(_._1).toSet))) { | |||
// if the new member does not support the group protocol, reject it | |||
responseCallback(joinError(memberId, Errors.INCONSISTENT_GROUP_PROTOCOL)) | |||
} else if (group.is(Empty) && protocols.isEmpty ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also check whether protocolType is non-empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a non-empty check on protocolType
@@ -146,6 +146,9 @@ class GroupCoordinator(val brokerId: Int, | |||
if (!group.is(Empty) && (!group.protocolType.contains(protocolType) || !group.supportsProtocols(protocols.map(_._1).toSet))) { | |||
// if the new member does not support the group protocol, reject it | |||
responseCallback(joinError(memberId, Errors.INCONSISTENT_GROUP_PROTOCOL)) | |||
} else if (group.is(Empty) && protocols.isEmpty ) { | |||
//reject if first member with empty group protocol | |||
responseCallback(joinError(memberId, Errors.INCONSISTENT_GROUP_PROTOCOL)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error code seems less than ideal, though I'm not sure there's a better alternative. Maybe we can at least extend the description in Errors
to cover this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the description of INCONSISTENT_GROUP_PROTOCOL error
@omkreddy By the way, did someone hit a problem with the current behavior or did you just notice the missing validation? |
a719a9d
to
7ce28b2
Compare
@hachikuji apparently one of our customers tried with null partition.assignment.strategy. I am not sure why they tried :) |
@omkreddy That's interesting. Seems like a validation gap on the client as well. Maybe when the user calls one of the |
7ce28b2
to
d40dd74
Compare
@hachikuji added a client-side validation check. Updated the PR |
|
||
if (assignors.isEmpty()) | ||
throw new IllegalArgumentException("Must specify at least one partition assigner"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use ConfigException here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a good question. IllegalArgumentException
doesn't feel quite right. Another option would be IllegalStateException
. Also, we should probably mention the configuration property in the exception message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hachikuji Updated the PR. Please check.
d40dd74
to
d7aa844
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor comments. The rest LGTM.
|
||
if (assignors.isEmpty()) | ||
throw new IllegalStateException("Must configure at least one partition assigner class name to" + | ||
" partition.assignment.strategy config"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we reference the property in ConsumerConfig
instead? Also, not sure it's worth it, but we could factor out a function, say throwIfNoAssignorsConfigured
to avoid duplicating the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good..Updated PR
@@ -266,7 +266,8 @@ public ApiException build(String message) { | |||
} | |||
}), | |||
INCONSISTENT_GROUP_PROTOCOL(23, | |||
"The group member's supported protocols are incompatible with those of existing members.", | |||
"The group member's supported protocols are incompatible with those of existing members" + | |||
" or first group member tried to join with empty protocol.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two cases, right? The protocol type might be empty, or the list of protocols might be empty.
d7aa844
to
56d21cf
Compare
@omkreddy Note the checkstyle error. |
@hachikuji ahh..sorry..fixed the check style issue. |
@omkreddy Since this is really a bug fix, would you mind creating a JIRA for this issue before I merge? |
…tocol type/protocol list
ae395db
to
1a16f0d
Compare
Raised JIRA for this : https://issues.apache.org/jira/browse/KAFKA-6005 |
Thanks for the patch. Merging to trunk. |
No description provided.