Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KAFKA-13406: skip assignment validation for built-in cooperativeStickyAssignor #11439

Merged
merged 3 commits into from
Nov 16, 2021
Merged

KAFKA-13406: skip assignment validation for built-in cooperativeStickyAssignor #11439

merged 3 commits into from
Nov 16, 2021

Conversation

showuon
Copy link
Contributor

@showuon showuon commented Oct 27, 2021 •
edited
Loading

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor. And also add tests.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@ableegoldman
Copy link
Contributor

@showuon something like this would be a good fix for 3.1, especially since it's quick and code freeze is coming up. I also feel it is low-risk, for one thing this validation clearly can't be trusted at the moment anyways, for another I think it's fair to trust the CooperativeStickyAssignor since we have many tests for it -- we could just move the validation check from the ConsumerCoordinator into the CooperativeStickyAssignor itself, but we essentially already do exactly that inside the adjustAssignment method. So I support merging this (once it's cleaned up) for 3.1 and then doing something like adding the generation to the ConsumerProtocol as you suggested for the long term fix

@showuon
Copy link
Contributor Author

showuon commented Oct 29, 2021

Oh, I didn't notice the v3.1.0 is going to code freeze soon. I thought I can still wait for the Andrei's test. Thanks for reminding, @ableegoldman . I'll clean it up today.

@ableegoldman
Copy link
Contributor

Well, "soon" -- I just saw the notice about feature freeze coming up, so it's not a rush exactly, but just wanted to give a heads up in case if you're like me and didn't see the 3.1 deadlines coming up because 3.0 seemed like it was only a few weeks ago 😅

@showuon showuon changed the title [WIP] KAFKA-13406: skip assignment validation for built-in cooperativeStickyAssignor KAFKA-13406: skip assignment validation for built-in cooperativeStickyAssignor Oct 29, 2021
@showuon
Copy link
Contributor Author

showuon commented Oct 29, 2021

@ableegoldman , it's good to review now. Thanks.

@showuon
Copy link
Contributor Author

showuon commented Nov 12, 2021

@ableegoldman , please take a look when available. Thank you.

ableegoldman
ableegoldman approved these changes Nov 16, 2021
View reviewed changes
Copy link
Contributor

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the delayed review

@ableegoldman
Copy link
Contributor

Failure is unrelated, merging to trunk. Will also need to cherrypick back to 3.1 cc @dajac

@ableegoldman ableegoldman merged commit 627cad1 into apache:trunk Nov 16, 2021
ableegoldman pushed a commit that referenced this pull request Nov 16, 2021
@showuon @ableegoldman
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
5eecb43 
…yAssignor (#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect 

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
ableegoldman pushed a commit that referenced this pull request Nov 16, 2021
@showuon @ableegoldman
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
18119c3 
…yAssignor (#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect 

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
@ableegoldman
Copy link
Contributor

Merged to trunk and ported back to 3.0 and 2.8, just waiting for David's approval before cherrypicking to the 3.1 branch

@dajac
Copy link
Member

dajac commented Nov 16, 2021

@ableegoldman Approved.

ableegoldman pushed a commit that referenced this pull request Nov 16, 2021
@showuon @ableegoldman
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
dee8245 
…yAssignor (#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect 

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
@ableegoldman
Copy link
Contributor

Thanks! Pushed to 3.1 as well

hgeraldino pushed a commit to hgeraldino/kafka that referenced this pull request Nov 29, 2021
@showuon @hgeraldino
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
73eab80 
…yAssignor (apache#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect 

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
hachikuji pushed a commit that referenced this pull request Dec 21, 2021
@showuon
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
6eed774 
…yAssignor (#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
hachikuji pushed a commit that referenced this pull request Dec 22, 2021
@showuon
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
c8d2983 
…yAssignor (#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
hachikuji pushed a commit that referenced this pull request Dec 22, 2021
@showuon
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
dc209f1 
…yAssignor (#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
@showuon
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
8afa06c 
…yAssignor (apache#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect 

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
lmr3796 pushed a commit to lmr3796/kafka that referenced this pull request Jun 2, 2022
@showuon @lmr3796
KAFKA-13406: skip assignment validation for built-in cooperativeStick…
a981e46 
…yAssignor (apache#11439)

This fix is trying to skip the assignment validation for built-in cooperative sticky assignor, since (a) we know the assignment is valid since we do essentially this same check already in the cooperative sticky assignor, and (b) the check is broken anyways due to potential for claimed `ownedPartitions` to be incorrect 

Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>
@dnwe dnwe mentioned this pull request Aug 23, 2023
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ableegoldman ableegoldman ableegoldman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@showuon @ableegoldman @dajac