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

Replace ClusterLogAllocation usage by ClusterInfoBuilder #1184

Conversation

garyparrot
Copy link
Collaborator

Resolve #1157.
Previous related PR #1165.

這個 PR 根據 #1165 實作出來的 ClusterInfoBuilder,在上面添加兩個函數,分別用來 reassign replica 和 change preferred leader,這些功能將會取代 ClusterLogAllocation

  • 新增 ClusterInfoBuilder#reassignReplica
  • 新增 ClusterInfoBuilder#setPreferredLeader
  • 刪除 ClusterLogAllocation
  • AllocationTweaker 現在接受和回傳 ClusterInfo<Replica>,內部用 ClusterInfoBuilder 來修改 ClusterInfo

@garyparrot garyparrot self-assigned this Nov 29, 2022
chia7712
chia7712 previously approved these changes Nov 29, 2022
Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@garyparrot 整體很棒,就一個小建議,修改完就可以合併了

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@garyparrot 剛剛又看了一下,對程式碼品質有一些想法,請看一下

@garyparrot garyparrot requested a review from chia7712 December 1, 2022 03:42
@garyparrot
Copy link
Collaborator Author

@chia7712 已修正所有項目,麻煩再看看,感謝

chia7712
chia7712 previously approved these changes Dec 1, 2022
Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM 這次重構的很棒!

就一個小地方修正一下就可以合併了

@garyparrot garyparrot merged commit f292ba0 into opensource4you:main Dec 1, 2022
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.

Proposing a way to alter ClusterInfo
2 participants