Skip to content

MINOR: Client state machine fix for transition to stable on initial empty assignment#15033

Merged
cadonna merged 5 commits intoapache:trunkfrom
lianetm:client-state-machine-fix-3
Dec 20, 2023
Merged

MINOR: Client state machine fix for transition to stable on initial empty assignment#15033
cadonna merged 5 commits intoapache:trunkfrom
lianetm:client-state-machine-fix-3

Conversation

@lianetm
Copy link
Copy Markdown
Member

@lianetm lianetm commented Dec 17, 2023

This includes a fix for ensuring that a member transitions to STABLE when joining a group, in the case where it receives an initial empty assignment.

cadonna
cadonna reviewed Dec 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @lianetm !

Here my feedback!

Comment thread clients/src/main/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImpl.java Outdated
Comment on lines 410 to 411
// This is the case where a member was RECONCILING an unresolved
// assignment that was removed by the broker in a following assignment.
Copy link
Copy Markdown
Member

@cadonna cadonna Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not completely correct anymore. It only describes one case of the two. Doesn't this comment duplicate the comment above the if-statement.
I really strongly recommend to not use too many inline comments. They are really hard to properly maintain as we can see here.

Copy link
Copy Markdown
Member Author

@lianetm lianetm Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the comment is a dup of the one above and was not accurate anymore, removed (agree with the suggestion of not overdoing the inline comments, noted)

Comment thread ...nts/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java
}

@Test
public void testMemberJoiningTransitionsToStableWhenReceivingEmptyAssignment() {
Copy link
Copy Markdown
Member

@cadonna cadonna Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there also a test for the second case? I skimmed over the test class but could not find one.

Copy link
Copy Markdown
Member Author

@lianetm lianetm Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is, testNewEmptyAssignmentReplacesPreviousOneWaitingOnMetadata covers the other case.

@lianetm
Copy link
Copy Markdown
Member Author

lianetm commented Dec 18, 2023

Thanks a lot for the review @cadonna . All comments addressed.

cadonna
cadonna approved these changes Dec 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cadonna
Copy link
Copy Markdown
Member

cadonna commented Dec 19, 2023

@lianetm
Are the following failures related?

Build / JDK 11 and Scala 2.13 / kafka.api.PlaintextConsumerTest.testShrinkingTopicSubscriptions(String, String).quorum=kraft+kip848.groupProtocol=consumer
Build / JDK 17 and Scala 2.13 / kafka.api.PlaintextConsumerTest.testExpandingTopicSubscriptions(String, String).quorum=kraft+kip848.groupProtocol=consumer
Build / JDK 17 and Scala 2.13 / kafka.api.PlaintextConsumerTest.testExpandingTopicSubscriptions(String, String).quorum=kraft+kip848.groupProtocol=consumer

@lianetm
Copy link
Copy Markdown
Member Author

lianetm commented Dec 19, 2023

@cadonna those are unrelated, they had been failing in other PRs as well and are now disabled (disabled with @philipnee latest PR that I just merged here along with trunk latest changes). Thanks!

@lianetm
Copy link
Copy Markdown
Member Author

lianetm commented Dec 20, 2023

Test failures seem unrelated. I checked around testCoordinatorFailover that, even though it passes locally, it failed on the CI for the new consumer, but it's been failing for other recent PRs and reported flaky with KAFKA-16024.

@cadonna cadonna merged commit db8af51 into apache:trunk Dec 20, 2023
cadonna pushed a commit that referenced this pull request Dec 20, 2023
@lianetm @cadonna
MINOR: Client state machine fix for transition to stable on initial e…
89de421 
…mpty assignment (#15033)

This includes a fix for ensuring that a member transitions to STABLE when joining a group, in the case where it receives an initial empty assignment.

Reviewer: Bruno Cadonna <cadonna@apache.org>
@philipnee philipnee added KIP-848 The Next Generation of the Consumer Rebalance Protocol ctr Consumer Threading Refactor (KIP-848) consumer labels Dec 21, 2023
gaurav-narula pushed a commit to gaurav-narula/kafka that referenced this pull request Jan 24, 2024
@lianetm @gaurav-narula
MINOR: Client state machine fix for transition to stable on initial e…
f679940 
…mpty assignment (apache#15033)

This includes a fix for ensuring that a member transitions to STABLE when joining a group, in the case where it receives an initial empty assignment.

Reviewer: Bruno Cadonna <cadonna@apache.org>
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
@lianetm @yyu1993
MINOR: Client state machine fix for transition to stable on initial e…
d5397b3 
…mpty assignment (apache#15033)

This includes a fix for ensuring that a member transitions to STABLE when joining a group, in the case where it receives an initial empty assignment.

Reviewer: Bruno Cadonna <cadonna@apache.org>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
@lianetm @AnatolyPopov
MINOR: Client state machine fix for transition to stable on initial e…
ed44715 
…mpty assignment (apache#15033)

This includes a fix for ensuring that a member transitions to STABLE when joining a group, in the case where it receives an initial empty assignment.

Reviewer: Bruno Cadonna <cadonna@apache.org>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
@lianetm @clolov
MINOR: Client state machine fix for transition to stable on initial e…
7360066 
…mpty assignment (apache#15033)

This includes a fix for ensuring that a member transitions to STABLE when joining a group, in the case where it receives an initial empty assignment.

Reviewer: Bruno Cadonna <cadonna@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cadonna cadonna cadonna approved these changes

Assignees

No one assigned

Labels

consumer ctr Consumer Threading Refactor (KIP-848) KIP-848 The Next Generation of the Consumer Rebalance Protocol

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lianetm @cadonna @philipnee