-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-5253. Support container move HA #2488
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
|
@lokeshj1703 @ChenSammi @siddhantsangwan PTAL! if the logics looks good , i will add more unit test in additional commit, thanks |
bbe9318 to
c7b0ef8
Compare
c7b0ef8 to
399bde3
Compare
lokeshj1703
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.
@JacksonYao287 Thanks for working on this PR! The changes look good. I have few minor comments inline.
...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
Outdated
Show resolved
Hide resolved
...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
Show resolved
Hide resolved
|
i have updated the PR according the comments, @lokeshj1703 @ChenSammi @siddhantsangwan please take a look! |
|
@JacksonYao287 Can you also add a UT with HA to test the consistency of move? |
@lokeshj1703 , sure, thanks, if the logic looks good to you now, i will add UT in a new commit |
|
@lokeshj1703 I have added UT and integration tests for move HA, PTAL! |
|
CI failures seems not caused by this patch, i will merge mater branch again after they are fixed! #2420 |
lokeshj1703
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.
@JacksonYao287 Thanks for updating the PR! I have few minor comments. +1 o.w.
...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
Show resolved
Hide resolved
...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
Show resolved
Hide resolved
|
Also we will need to make sure that balancer is always restarted on leader with the new configuration. This would be a separate PR. |
sure , will do this |
|
@JacksonYao287 We will also need to add timeout for move with the leader changes. After a leader change the move will be reset so I think we will need to handle the timeouts as well. It might require some thought. Please see if you would like to address it in this PR. |
thanks @lokeshj1703 for pointing out this. i think it is better off addressing it in a new jira, will do it later |
|
@JacksonYao287 Thanks for the contribution! I have committed the PR to master branch. |
|
thanks @lokeshj1703 for the review! |
What changes were proposed in this pull request?
make balancer HA aware
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5253
How was this patch tested?
unit test