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

move movementConstraint to MoveCost #1531

Merged
merged 24 commits into from
Mar 12, 2023

Conversation

qoo332001
Copy link
Collaborator

此PR根據comment 把成本估計限制移動到各自的成本估計實做中並套用到BalancerHandler中,如此一來在Balancer端就不需要在意每個成本是如何做限制的

@qoo332001 qoo332001 requested a review from chia7712 March 3, 2023 07:05
@qoo332001 qoo332001 requested a review from chia7712 March 4, 2023 03:36
@qoo332001 qoo332001 requested a review from chia7712 March 6, 2023 13:14
@qoo332001 qoo332001 requested a review from chia7712 March 7, 2023 03:01
@qoo332001 qoo332001 requested a review from chia7712 March 7, 2023 13:38
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.

@qoo332001 PR的概念很棒,一些細節再調整一下就可以合併了

@chia7712
Copy link
Contributor

chia7712 commented Mar 8, 2023

@qoo332001 這隻看能不能先完成,趁著這隻把 constraint 的部分功能移到 move cost 後,我們可以接著後面做 balancer benchmark 看看套上簡單的 constraint (例如 leader 數量限制),看看plan的效率會變得怎樣

@chia7712
Copy link
Contributor

chia7712 commented Mar 8, 2023

麻煩rebase喔

@qoo332001 qoo332001 requested a review from chia7712 March 11, 2023 09:17
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.

@qoo332001 感謝更新,還有幾個點可以再調整一下

@qoo332001 qoo332001 requested a review from chia7712 March 11, 2023 13:22
@qoo332001 qoo332001 requested a review from chia7712 March 12, 2023 17:54
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.

@qoo332001 感謝,整體看起來不錯。不過後續還有一個要接著討論的設計議題:MoveCost是否還需要保有“列舉”般的介面?

我們盡量不在這些動態的物件上做“列舉”,這是為了日後的相容性,同時避免使用端 (balacner, assingor) 看到太多“細節”。當時為了讓MoveCost可以作為constraint,我們第一版用了通用的介面 (unit, value 等等),但使用端必須用switch-case列舉的方式逐一檢查,第二版我們將列舉移動到的MoveCost上,讓各個cost function去填寫適合自己的列舉,然後使用端就拿需要的數值去計算。

現在這一版,你新增了overflow這個“純值”,所以balancer可以不需要列舉了,一切都透過參數控制,那麼是不是MoveCost也可以擺脫列舉?例如,使用端要產生"move cost" 報告的話,我們是否有可以提供簡單的前後 cluster info 比較直接產生所有報表?又或者我們定義一個簡單的通用格式 (broker id -> string) 讓Move cost 去簡單填寫就好?

這個討論麻煩開個議題,然後試著講一下你的想法。謝謝

@qoo332001 qoo332001 merged commit 0516739 into opensource4you:main Mar 12, 2023
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