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
HDDS-8170. Let ContainerBalancer consider EC containers for balancing #4542
Conversation
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 @siddhantsangwan for the patch.
boolean legacyEnabled = ozoneConfiguration. | ||
getBoolean("hdds.scm.replication.enable.legacy", true); |
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 same config can be looked up by replicationManager.getConfig().isLegacyEnabled()
. That way we could avoid the need to pass OzoneConfiguration
to the constructor, as well as the duplication of config key name.
Nit: I think we can also skip storing this in a local variable.
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.
Okay, I'm using replicationManager.getConfig().isLegacyEnabled()
in this class and ContainerBalancerTask
as well now. Changed the unit tests accordingly.
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 @siddhantsangwan for updating the patch.
@@ -49,20 +50,23 @@ public class ContainerBalancerSelectionCriteria { | |||
private Set<ContainerID> selectedContainers; | |||
private Set<ContainerID> excludeContainers; | |||
private FindSourceStrategy findSourceStrategy; | |||
private OzoneConfiguration ozoneConfiguration; |
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.
nit: this member and the corresponding constructor parameter are no longer needed.
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.
Oh, thought I removed those already. Anyway I've removed them now.
cc @aswinshakil @swamirishi can you'll please take a look? |
What changes were proposed in this pull request?
This PR enables balancing EC containers in
ContainerBalancer
. SinceMoveManager
has been integrated with ContainerBalancer now, we can support balancing EC containers.Changes include:
ContainerBalancerSelectionCriteria
only if Legacy RM is enabled.DEBUG
logs inMoveManager
.ContainerBalancer
where it was logging aDuration
instead of the long value of a config.TestContainerBalancerTask
to also create EC containers along with the RATIS containers it was already creating. Change some tests accordingly.TestContainerBalancerTask
, except in specific tests where we're testing both Legacy RM andMoveManager
.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8170
How was this patch tested?
Green CI in my fork: https://github.com/siddhantsangwan/ozone/actions/runs/4617711869
Added a UT and did some docker-compose manual testing.
Screenshot showing an EC container with id 6 and replica index 4 being replicated by balancer:
Screenshot showing the same container being deleted: