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 MigrateCost to MoveCost #1585

Merged

Conversation

qoo332001
Copy link
Collaborator

comment 將MigrationCost移動到CostFunction,如此一來Balancer就不需知道當前有哪些成本要計算,以及列舉這些成本

@chia7712
Copy link
Contributor

@qoo332001 好像連 build 都不會過,可以看一下嗎

@qoo332001
Copy link
Collaborator Author

qoo332001 commented Mar 22, 2023

@qoo332001 好像連 build 都不會過,可以看一下嗎

已更新,麻煩再看一下

@qoo332001 qoo332001 requested a review from chia7712 March 22, 2023 08:27
@chia7712
Copy link
Contributor

@qoo332001 可否試著說明一下為何選擇這個作法?

@qoo332001
Copy link
Collaborator Author

@qoo332001 可否試著說明一下為何選擇這個作法?

實做時主要是考慮兩個作法:

  1. 新增一個方法傳入前後ClusterInfo來一次計算出所有成本
  2. 讓各個CostFunction個別實做要怎麼計算成本

直覺上當使用者在自行新增(實做)CostFunction時,可以同時實做成本的計算方法會比較簡單也方便,若是每新增一個CostFunction都要去更改1.那個方法,那會導致CostFunction實做上非常不方便

@chia7712
Copy link
Contributor

@qoo332001 感謝回覆,不過我還想了解一下MigrationCost所夾帶的資訊你預期還有哪些應用場景?我分享一個把MigrationCost放回介面的擔憂:move cost/cluster cost 都是屬於會不斷被呼叫的使用方式 ( partition cost/broker cost 則是久久被呼叫一次),所以介面的定義要稍微在意一下成本,所以如果我們把這種篤定要窮舉才能計算出來的結構放在介面上,就會變向的限制 cost function 的演算法設計(例如在意總搬移量的成本,可以只算到超過就立刻回傳,而不需要全部算完)

當然上述擔憂也可能算是 overkill,所以我想聽聽你預期的使用場景

@qoo332001
Copy link
Collaborator Author

所以我想聽聽你預期的使用場景

現在的用法就只是最後在搬移計畫確定後,會想要讓使用者知道搬移成本,當初會想要放到介面主要是怕未來會有調用成本的需求,所以才包成MigrationCost,若是包成介面有成本的考量,或許現在可以先改成用String來替代MigrationCost?

@chia7712
Copy link
Contributor

若是包成介面有成本的考量,或許現在可以先改成用String來替代MigrationCost?

我說的成本是指“演算法本來可以 return early 的地方,會因為要產生一個“完整”的報告,導致必須全部算完

例如 #1584 中要去限制最大的移動量,當叢集很大的時候,我們可以做一個優化叫做“統計到超過限制時就提早回傳 overflow“,如此剩下的 partitions 就可以不用統計了,因為已經 overflow 了,但是如果現在在介面上加上說要傳回“統計報告”,這樣就會讓實作一定要“看完”所有的 partitions

@qoo332001
Copy link
Collaborator Author

我們可以做一個優化叫做“統計到超過限制時就提早回傳 overflow“

我將這個想法更新在 #1584ReplicaLeaderSizeCostRecordSizeCost ,目前的作法是每當計算完一個broker的搬移資料量後,確認一次有沒有overflow,若有就提早離開並回傳overflow

但是如果現在在介面上加上說要傳回“統計報告”,這樣就會讓實作一定要“看完”所有的 partitions

#1584 合併後,或許可以把這段移出介面,改成在ClusterInfo中做一個靜態方法,一次算出所有成本

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 感謝更新

現在有許多 helpers 要稍微思考和慎選一下要放的位置,才不會讓程式碼之間互相依賴的狀況變複雜

@qoo332001 qoo332001 requested a review from chia7712 April 12, 2023 07:34
chia7712
chia7712 previously approved these changes Apr 12, 2023
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 基本沒什麼問題了,一個小建議調整後就可以合併了

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

@qoo332001 qoo332001 merged commit f353fca into opensource4you:main Apr 13, 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