Skip to content

MINOR: Update the kafka-reassign-partitions script command in documentation#12237

Merged
showuon merged 2 commits intoapache:trunkfrom
RivenSun2:minor-reassign
Jun 2, 2022
Merged

MINOR: Update the kafka-reassign-partitions script command in documentation#12237
showuon merged 2 commits intoapache:trunkfrom
RivenSun2:minor-reassign

Conversation

@RivenSun2
Copy link
Copy Markdown
Contributor

@RivenSun2 RivenSun2 commented Jun 2, 2022

As mentioned in Notable changes in 2.6.0 : There are several notable changes to the reassignment tool kafka-reassign-partitions.sh following the completion of KIP-455. This tool now requires the --additional flag to be provided when changing the throttle of an active reassignment.

It is recommended that the actual operation guidance document should also be updated to avoid causing confusion to users. Otherwise, following the original documentation, the user will see an exception:
Cannot execute because there is an existing partition assignment. Use --additional to override this and create a new partition assignment in addition to the existing one. The --additional flag can also be used to change the throttle by resubmitting the current reassignment.

Committer Checklist (excluded from commit message)

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

@RivenSun2
Copy link
Copy Markdown
Contributor Author

Hi @showuon and @dajac
Could you help review this PR?
Thanks.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@RivenSun2 , thanks for the PR. Yes, this is a bug that I've already addressed in another PR. (but got no response :)). So, let's fix it here! In my PR, I also found the command output is different now. I saw you fix some (add "inter-broker"), but from my previous observation (3 months ago), there are still some other places needed to be updated. Could you help confirm them?

Thank you.

@RivenSun2
Copy link
Copy Markdown
Contributor Author

@showuon ok i will compare carefully and revise to latest correct output.
Thanks.

if (state.done) {
if (state.currentReplicas.equals(state.targetReplicas)) {
bld.append("Reassignment of partition %s is complete.".
bld.append("Reassignment of partition %s is completed.".
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adjust the output here.

Comment thread docs/ops.html
Successfully started reassignment of partitions
{"version":1,
"partitions":[{"topic":"foo","partition":0,"replicas":[5,6,7]}]}</code></pre>
Successfully started partition reassignment for foo-0</code></pre>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because there is only one topicPartition, output reassignment instead of reassignments

Comment thread docs/ops.html
Comment on lines +387 to +391
Reassignment of partition [my-topic,1] is completed
Reassignment of partition [my-topic,0] is completed

Clearing broker-level throttles on brokers 1,2,3
Clearing topic-level throttles on topic my-topic</code></pre>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

According to the actual output result, there will be a blank line in the middle

@RivenSun2
Copy link
Copy Markdown
Contributor Author

Hi @showuon
I just resubmitted the code, please help to check.
Thanks.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! I'm going to close my PR then. Thank you.

@showuon
Copy link
Copy Markdown
Member

showuon commented Jun 2, 2022

Waiting for the jenkins build since we have a output change in ReassignPartitionsCommand.scala

@RivenSun2
Copy link
Copy Markdown
Contributor Author

The failed testcase:testTopicIdUpgradeAfterReassigningPartitions(), should not be related to this PR, this testcase seems unstable.
In JDK8, an exception is thrown at ControllerIntegrationTest.scala:1475;
In JDK17 an exception is thrown at ControllerIntegrationTest.scala:1474.

@RivenSun2
Copy link
Copy Markdown
Contributor Author

RivenSun2 commented Jun 2, 2022

The same error also appeared in a merged PR a few days ago:
https://github.com/apache/kafka/pull/12222/checks?check_run_id=6632831283

and another PR that doesn't contain changes from this PR:
https://github.com/apache/kafka/pull/12226/checks?check_run_id=6648879431

@showuon
Copy link
Copy Markdown
Member

showuon commented Jun 2, 2022

Yes, the KRaftClusterTest failed tests are tracked in this PR: #12238. And testTopicIdUpgradeAfterReassigningPartitions is tracked in another PR: #11687. Thank you.

@showuon showuon changed the title MINOR: Update the documentation that modifies the throttle of an active reassignment MINOR: Update the kafka-reassign-partitions script command in documentation Jun 2, 2022
@showuon showuon merged commit d8d92f0 into apache:trunk Jun 2, 2022
@showuon
Copy link
Copy Markdown
Member

showuon commented Jun 4, 2022

@RivenSun2 , could you also file a PR to kafka-site repo to fix the current doc? Thanks.

@RivenSun2
Copy link
Copy Markdown
Contributor Author

Hi @showuon my pleasure. But I don't know how I need to do it?

@showuon
Copy link
Copy Markdown
Member

showuon commented Jun 6, 2022

Hi @showuon my pleasure. But I don't know how I need to do it?

What we fixed will not be reflected to the doc link until next release. To fix the issue in the current doc link: (ex: here), we should fix in the kafka-site repo. You can take this PR for reference: apache/kafka-site#414.

@RivenSun2
Copy link
Copy Markdown
Contributor Author

Ok thanks for the suggestion, I'll create a PR in that project later.

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