Skip to content

MINOR; Clean up LeaderAndIsrResponse construction in ReplicaManager#becomeLeaderOrFollower#10234

Merged
dajac merged 4 commits into
apache:trunkfrom
dajac:minor-refactor-in-replica-manager
Mar 4, 2021
Merged

MINOR; Clean up LeaderAndIsrResponse construction in ReplicaManager#becomeLeaderOrFollower#10234
dajac merged 4 commits into
apache:trunkfrom
dajac:minor-refactor-in-replica-manager

Conversation

@dajac

@dajac dajac commented Mar 1, 2021

Copy link
Copy Markdown
Member

This patch refactors the code, which constructs the LeaderAndIsrResponse in ReplicaManager#becomeLeaderOrFollower, to improve the readability and to remove unnecessary operations.

Committer Checklist (excluded from commit message)

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

@jolshan jolshan 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.

Thanks for the PR. This really cleans up the code. LGTM

@dajac
dajac marked this pull request as ready for review March 2, 2021 09:44

@chia7712 chia7712 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.

+1 if the following question is invalid :)

Comment thread clients/src/main/java/org/apache/kafka/common/requests/LeaderAndIsrRequest.java Outdated
@dajac

dajac commented Mar 3, 2021

Copy link
Copy Markdown
Member Author

@chia7712 Thanks for the review. I just pushed a commit to address your comment. Could you take another look?

@chia7712 chia7712 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.

+1 to this nice patch. nit: The topic name is a bit mysterious to me when reading so it would be nice to add some comments :)

Comment thread core/src/main/scala/kafka/server/ReplicaManager.scala
@dajac

dajac commented Mar 3, 2021

Copy link
Copy Markdown
Member Author

@chia7712 I just pushed another small commit to fix/improve LeaderAndIsrRequestTest#testGetErrorResponse. It was not testing all the versions as claimed. I have also added assertions to verify the created response. Could you take a look?

@chia7712 chia7712 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.

+1 again. one question unrelated to this PR is left :)

@dajac

dajac commented Mar 4, 2021

Copy link
Copy Markdown
Member Author

@chia7712 I just pushed a commit to address your comment. Could you take a look?

@chia7712 chia7712 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.

LGTM

@dajac
dajac merged commit 3ef39e1 into apache:trunk Mar 4, 2021
@dajac
dajac deleted the minor-refactor-in-replica-manager branch March 4, 2021 09:31
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