Skip to content

KAFKA-5929: Save pre-assignment to file to avoid too long text to display when do topic partition reassign#3897

Closed
uncleGen wants to merge 3 commits into
apache:trunkfrom
uncleGen:KAFKA-5929
Closed

KAFKA-5929: Save pre-assignment to file to avoid too long text to display when do topic partition reassign#3897
uncleGen wants to merge 3 commits into
apache:trunkfrom
uncleGen:KAFKA-5929

Conversation

@uncleGen

@uncleGen uncleGen commented Sep 19, 2017

Copy link
Copy Markdown
Contributor

When do partition reassign

  • before pr
    Pre-assignment will be printed directly. It is not friendly when the text is too long.

  • after pr
    Pre-assignment will still be printed directly, but will be save to a file at the same time, naming with suffix ".rollback" of "reassignment-json-file". For example:

./kafka-reassign-partitions.sh --reassignment-json-file test.json ...

then we may get a file test.json.rollback

@uncleGen

Copy link
Copy Markdown
Contributor Author

retest this please

@uncleGen

Copy link
Copy Markdown
Contributor Author

ready for review

@uncleGen uncleGen closed this Sep 25, 2017
@uncleGen uncleGen reopened this Sep 27, 2017
@uncleGen

Copy link
Copy Markdown
Contributor Author

@lindong28 Could you please take a review?

@asfgit

asfgit commented Nov 15, 2017

Copy link
Copy Markdown

FAILURE
8067 tests run, 5 skipped, 2 failed.
--none--

1 similar comment
@asfgit

asfgit commented Nov 15, 2017

Copy link
Copy Markdown

FAILURE
8067 tests run, 5 skipped, 2 failed.
--none--

@asfgit

asfgit commented Nov 15, 2017

Copy link
Copy Markdown

SUCCESS
8067 tests run, 5 skipped, 0 failed.
--none--

@lindong28 lindong28 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The patch LGTM overall. Left one comment.

I think this is a improvement but not that necessary. Usually I would re-direct output via pipe to another file. Committer can decide whether to accept or not.

}

def executeAssignment(zkUtils: ZkUtils, adminClientOpt: Option[JAdminClient], reassignmentJsonString: String, throttle: Throttle, timeoutMs: Long = 10000L) {
def executeAssignment(zkUtils: ZkUtils, adminClientOpt: Option[JAdminClient], reassignmentJsonString: String, reassignmentJsonFile: String, throttle: Throttle, timeoutMs: Long = 10000L) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

making reassignmentJsonFile an Option[String] is probably better than using empty to indicate that it is not used.

@uncleGen uncleGen Nov 16, 2017

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.

get it, thanks for your review!

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.

3 participants