MINOR: Fix partition loading checks in GroupCoordinator#4788
Merged
hachikuji merged 2 commits intoapache:trunkfrom Apr 2, 2018
Merged
MINOR: Fix partition loading checks in GroupCoordinator#4788hachikuji merged 2 commits intoapache:trunkfrom
hachikuji merged 2 commits intoapache:trunkfrom
Conversation
hachikuji
changed the title
guozhangwang
approved these changes
Mar 29, 2018
Contributor
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
Only a couple minor comments. Please feel free to merge after that.
| groupManager.getGroup(groupId) match { | ||
| responseCallback: SyncCallback): Unit = { | ||
| validateGroup(groupId) match { | ||
| case Some(error) if error == Errors.COORDINATOR_LOAD_IN_PROGRESS => |
Contributor
There was a problem hiding this comment.
Since we are handling COORDINATOR_LOAD_IN_PROGRESS on the client side now, could we just return this error code than translating it to REBALANCE_IN_PROGRESS?
Contributor
Author
There was a problem hiding this comment.
I wanted to do that, but it would break older clients. Maybe we can leave a note somewhere to do this in the next protocol bump? I guess we could leave out the client handling of the error until then. What do you think?
| case Some(error) => responseCallback(Array.empty, error) | ||
|
|
||
| case None => | ||
| groupManager.getGroup(groupId) match { |
Contributor
There was a problem hiding this comment.
The indention below seems not aligned?
3 tasks
hachikuji
added a commit
that referenced
this pull request
Apr 11, 2018
…pId (#4851) We had a regression in #4788 which caused the offset commit/fetch/describe APIs to fail if the groupId was empty. This should be allowed for backwards compatibility. Additionally, I have modified DeleteGroups to allow removal of the empty group, which was missed in the initial implementation. I've added a test case to ensure that we do not miss this again in the future. Reviewers: Ismael Juma <ismael@juma.me.uk>
koqizhao
pushed a commit
to koqizhao/kafka
that referenced
this pull request
Apr 12, 2018
…pId (apache#4851) We had a regression in apache#4788 which caused the offset commit/fetch/describe APIs to fail if the groupId was empty. This should be allowed for backwards compatibility. Additionally, I have modified DeleteGroups to allow removal of the empty group, which was missed in the initial implementation. I've added a test case to ensure that we do not miss this again in the future. Reviewers: Ismael Juma <ismael@juma.me.uk>
koqizhao
pushed a commit
to koqizhao/kafka
that referenced
this pull request
Apr 12, 2018
…pId (apache#4851) We had a regression in apache#4788 which caused the offset commit/fetch/describe APIs to fail if the groupId was empty. This should be allowed for backwards compatibility. Additionally, I have modified DeleteGroups to allow removal of the empty group, which was missed in the initial implementation. I've added a test case to ensure that we do not miss this again in the future. Reviewers: Ismael Juma <ismael@juma.me.uk>
koqizhao
pushed a commit
to koqizhao/kafka
that referenced
this pull request
Apr 12, 2018
…pId (apache#4851) We had a regression in apache#4788 which caused the offset commit/fetch/describe APIs to fail if the groupId was empty. This should be allowed for backwards compatibility. Additionally, I have modified DeleteGroups to allow removal of the empty group, which was missed in the initial implementation. I've added a test case to ensure that we do not miss this again in the future. Reviewers: Ismael Juma <ismael@juma.me.uk>
ying-zheng
pushed a commit
to ying-zheng/kafka
that referenced
this pull request
Jul 6, 2018
…pId (apache#4851) We had a regression in apache#4788 which caused the offset commit/fetch/describe APIs to fail if the groupId was empty. This should be allowed for backwards compatibility. Additionally, I have modified DeleteGroups to allow removal of the empty group, which was missed in the initial implementation. I've added a test case to ensure that we do not miss this again in the future. Reviewers: Ismael Juma <ismael@juma.me.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the group coordinator, we currently we check whether the partition is owned before checking whether it is loading. Since loading is a prerequisite for partition ownership, it means that it is not actually possible to see the COORDINATOR_LOAD_IN_PROGRESS error. The impact is mostly harmless: while loading the group, the client may send unnecessary FindCoordinator requests to rediscover the coordinator. I've fixed the bug and restructured the code to enable testing.
In the process of fixing this bug, I made the following minor changes:
Committer Checklist (excluded from commit message)