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.

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

MINOR: update doc for default assignor change #11009

Merged
merged 3 commits into from
Jul 13, 2021

Conversation

showuon
Copy link
Contributor

@showuon showuon commented Jul 9, 2021

Update the doc and upgrade doc for default assignor change.
REF: #10903

Committer Checklist (excluded from commit message)

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

@showuon
Copy link
Contributor Author

showuon commented Jul 9, 2021

@ableegoldman , I found I forgot to update the doc to reflect the default assignor change. Please take a look. Thanks.

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.

One small suggestion to clarify a bit further, otherwise looks good. Thanks for the followup, I also forgot about the docs 😅

Comment on lines 124 to 125
"<p>The default assignor is [RangeAssignor, CooperativeStickyAssignor], which will use RangeAssignor to do assignment, " +
"but just needs 1 rolling bounce to upgrade to CooperativeStickyAssignor</p>" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"<p>The default assignor is [RangeAssignor, CooperativeStickyAssignor], which will use RangeAssignor to do assignment, " +
"but just needs 1 rolling bounce to upgrade to CooperativeStickyAssignor</p>" +
"<p>The default assignor is [RangeAssignor, CooperativeStickyAssignor], which will use the RangeAssignor by default, " +
"but allows upgrading to the CooperativeStickyAssignor with just a single rolling bounce that removes the RangeAssignor from the list</p>" +

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, give me a ping when the build is finished

@showuon
Copy link
Contributor Author

showuon commented Jul 13, 2021

Previous build has one test group timeout. Re-trigger it again.

@ableegoldman
Copy link
Contributor

Seeing as these are just docs changes and at least one build passed completely while others just had a handful of flaky tests fail, I think we can go ahead and merge this

@ableegoldman ableegoldman merged commit ac18bdc into apache:trunk Jul 13, 2021
ableegoldman pushed a commit that referenced this pull request Jul 13, 2021
Update the doc and upgrade doc for default assignor change. REF: #10903

Reviewers: Anna Sophie Blee-Goldman <[email protected]>
@ableegoldman
Copy link
Contributor

Merged to trunk and cherrypicked to 3.0

xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
Update the doc and upgrade doc for default assignor change. REF: apache#10903

Reviewers: Anna Sophie Blee-Goldman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants