-
Notifications
You must be signed in to change notification settings - Fork 587
HDDS-9345. Add CapacityPipelineChoosePolicy considering datanode storage space #5354
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
Conversation
|
Please add some description to the PR about how this new policy would work, why it is needed etc. |
Added at the beginning of this page |
siddhantsangwan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall idea looks good to me. I have some comments below. Haven't checked the tests yet.
| targetPipeline = | ||
| !metric1.isGreater(metric2.get()) ? pipeline1 : pipeline2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible that we're checking two pipelines which share a datanode (multi raft), and that datanode is the most used one in both the pipelines. This will result in a tie and we'll choose the first pipeline. I'm wondering if it's better to break the tie by comparing the second most used node in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible that we're checking two pipelines which share a datanode (multi raft), and that datanode is the most used one in both the pipelines. This will result in a tie and we'll choose the first pipeline. I'm wondering if it's better to break the tie by comparing the second most used node in that case.
@siddhantsangwan Thanks for review ! It's a good idea to consider a second node. I'll update the code later.
( Also, I'm thinking there shouldn't be a need to consider a third node, as that might make the logic quite redundant. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased master branch and add a commit "add second compare logic". I tested in test env and debug log is printed as follows.
@siddhantsangwan If you have the time, PTAL again. Thanks!
|
debug log as follows: Format the above log for easy analysis: Meet expectations. |
xichen01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whbing The change looks good. Just a few comment you can refer to.
...java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/CapacityPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public Pipeline choosePipeline(List<Pipeline> pipelineList, | ||
| PipelineRequestInformation pri) { | ||
| Pipeline pipeline1 = healthPolicy.choosePipeline(pipelineList, pri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some Cluster, There's maybe close hundred pipelines. We just compare two Pipeline in here.
Does this make the probability of the largest (in capacity) Pipeline being selected low?
Perhaps a possible solution is to add a configuration that determines how many Pipelines are compared at a time, which takes the value [0, 1]
- When it is 0, only one Pipeline is selected at a time, which is basically equivalent to the
RandomPipelineChoosePolicy. - When 1, it compares all Pipelines, and strictly chooses the largest Pipeline in the whole world.
PS: But even if this feature needs to be implemented, I think it can be done in another PR, and when this PR is merged, the current solution will work in a small cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some Cluster, There's maybe close hundred pipelines. We just compare two
Pipelinein here. Does this make the probability of the largest (in capacity) Pipeline being selected low?Perhaps a possible solution is to add a configuration that determines how many Pipelines are compared at a time, which takes the value [0, 1]
- When it is 0, only one Pipeline is selected at a time, which is basically equivalent to the
RandomPipelineChoosePolicy.- When 1, it compares all Pipelines, and strictly chooses the largest Pipeline in the whole world.
PS: But even if this feature needs to be implemented, I think it can be done in another PR, and when this PR is merged, the current solution will work in a small cluster.
@xichen01 Thanks for review ! About the logic of selection, there are links to this original papers in HDFS-11564. The algorithms of choosing 2 random nodes and then placing the container on the lower utilization node is discussed in great depth in this survey paper.
https://pdfs.semanticscholar.org/3597/66cb47572028eb70c797115e987ff203e83f.pdf
In addition, SCMContainerPlacementCapacity#chooseNode also uses this algorithm. So, I wonder if it is not necessary to find the pipeline with minimum storage every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any test result for this algo comparing with random healthy node policy? just to see effectivness of algo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xichen01 Thanks for review ! About the logic of selection, there are links to this original papers in HDFS-11564. The algorithms of choosing 2 random nodes and then placing the container on the lower utilization node is discussed in great depth in this survey paper.
https://pdfs.semanticscholar.org/3597/66cb47572028eb70c797115e987ff203e83f.pdf
In addition, SCMContainerPlacementCapacity#chooseNode also uses this algorithm. So, I wonder if it is not necessary to find the pipeline with minimum storage every time?
@whbing Understood. For a fairly balanced cluster, such as a new one, this strategy can work very well, providing similar loads to all DataNodes.
However, for a significantly unbalanced cluster, like when adding new nodes, this strategy might be limited, especially in larger clusters.
But for the latter case (adding new nodes), we can also balance it using a balancer
|
ci passed in my branch https://github.com/whbing/ozone/actions/runs/6956936286. |
sumitagrawl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whbing Thanks for working over this, IMO looks another approach to replace Random policy. Have few query...
...java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/CapacityPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/CapacityPipelineChoosePolicy.java
Show resolved
Hide resolved
| @Override | ||
| public Pipeline choosePipeline(List<Pipeline> pipelineList, | ||
| PipelineRequestInformation pri) { | ||
| Pipeline pipeline1 = healthPolicy.choosePipeline(pipelineList, pri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any test result for this algo comparing with random healthy node policy? just to see effectivness of algo.
|
@whbing thanks for you update, LGTM +1 |
|
Run test, got selected result like: |
sumitagrawl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whbing others LGTM,
are we planning to make policy as default? or its just an option provided. We may need have in defined docs.
@sumitagrawl an option provided and NOT change the default value. Add description in |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @whbing for working on this.
.../java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/PipelineChoosePolicyFactory.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/TestCapacityPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/TestCapacityPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/TestCapacityPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/RandomPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/HealthyPipelineChoosePolicy.java
Outdated
Show resolved
Hide resolved
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @whbing for updating the patch, LGTM.
| default PipelineChoosePolicy init(final NodeManager nodeManager) { | ||
| // override if the policy requires nodeManager | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
@siddhantsangwan @sodonnel please take another look |
siddhantsangwan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Does anyone else have any comments? Let's get this committed, it's been open for a long time now.
|
Thanks @whbing for the patch, @siddhantsangwan, @sodonnel, @sumitagrawl, @xichen01 for the review. |
…age space (apache#5354) (cherry picked from commit 73e6f90)
What changes were proposed in this pull request?
Add pipeline choose policy impl CapacityPipelineChoosePolicy.
Consider the following scenario:
Our cluster often scales up with new nodes, but the old nodes may already be quite full in terms of writes. The balancer's speed is usually slow, so it's essential to choose nodes with lower usage as much as possible.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9345
How was this patch tested?
dn storage info now:
The debug log indicates that the node with lower storage rate is selected when selecting pipeline with this pr: