You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/05/25 07:56:42 UTC
[GitHub] [ozone] JacksonYao287 opened a new pull request, #3455: HDDS-6533. support balancing EC container
JacksonYao287 opened a new pull request, #3455:
URL: https://github.com/apache/ozone/pull/3455
## What changes were proposed in this pull request?
support balancing EC container
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6533
## How was this patch tested?
current UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1198897746
Thanks @JacksonYao287 for the explanation! The problem is currently move API is tightly related to container processing logic in LegacyRM. Please see updateMoveIfNeeded function in LegacyRM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1199096412
@siddhantsangwan @lokeshj1703 thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 merged pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 merged PR #3455:
URL: https://github.com/apache/ozone/pull/3455
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1197904317
I think there is a new RM being added as part of EC. Please refer https://issues.apache.org/jira/browse/HDDS-6589.
I think the handling of over and under replicated containers is different in EC. Since move relies on the RM handling of over-replicated containers, logic should be affected for move in case of EC containers. Please correct me if I am wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a diff in pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on code in PR #3455:
URL: https://github.com/apache/ozone/pull/3455#discussion_r929645152
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/AbstractFindTargetGreedy.java:
##########
@@ -45,7 +44,7 @@
public abstract class AbstractFindTargetGreedy implements FindTargetStrategy {
private Logger logger;
private ContainerManager containerManager;
- private PlacementPolicy placementPolicy;
+ private PlacementPolicyValidateProxy placementPolicyValidateProxy;
Review Comment:
look good, will update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1195437998
thanks @lokeshj1703 for the review, i have updated this patch , PTAL!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1199068562
I see. Thanks @JacksonYao287 for sharing the jira links and the explanation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3455:
URL: https://github.com/apache/ozone/pull/3455#discussion_r891159623
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/AbstractFindTargetGreedy.java:
##########
@@ -162,9 +165,16 @@ private boolean containerMoveSatisfiesPlacementPolicy(
.filter(datanodeDetails -> !datanodeDetails.equals(source))
.collect(Collectors.toList());
replicaList.add(target);
- ContainerPlacementStatus placementStatus =
- placementPolicy.validateContainerPlacement(replicaList,
- containerInfo.getReplicationConfig().getRequiredNodes());
+ ContainerPlacementStatus placementStatus;
+ switch (containerInfo.getReplicationType()) {
+ case EC:
+ placementStatus = ecPlacementPolicy.validateContainerPlacement(
+ replicaList, containerInfo.getReplicationConfig().getRequiredNodes());
+ break;
+ default:
+ placementStatus = placementPolicy.validateContainerPlacement(replicaList,
+ containerInfo.getReplicationConfig().getRequiredNodes());
+ }
Review Comment:
Instead of deciding which policy to use in ContainerBalancer, would it be better if we decide this in the SCM or someplace else? That way we won't have to keep on adding a new switch case here if more policies are introduced in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a diff in pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on code in PR #3455:
URL: https://github.com/apache/ozone/pull/3455#discussion_r891039377
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/AbstractFindTargetGreedy.java:
##########
@@ -162,9 +165,16 @@ private boolean containerMoveSatisfiesPlacementPolicy(
.filter(datanodeDetails -> !datanodeDetails.equals(source))
.collect(Collectors.toList());
replicaList.add(target);
- ContainerPlacementStatus placementStatus =
- placementPolicy.validateContainerPlacement(replicaList,
- containerInfo.getReplicationConfig().getRequiredNodes());
+ ContainerPlacementStatus placementStatus;
+ switch (containerInfo.getReplicationType()) {
+ case EC:
+ placementStatus = ecPlacementPolicy.validateContainerPlacement(
+ replicaList, containerInfo.getReplicationConfig().getRequiredNodes());
+ break;
+ default:
+ placementStatus = placementPolicy.validateContainerPlacement(replicaList,
+ containerInfo.getReplicationConfig().getRequiredNodes());
+ }
Review Comment:
Better to move it to a separate function.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/AbstractFindTargetGreedy.java:
##########
@@ -55,10 +56,12 @@ public abstract class AbstractFindTargetGreedy implements FindTargetStrategy {
protected AbstractFindTargetGreedy(
ContainerManager containerManager,
PlacementPolicy placementPolicy,
+ PlacementPolicy ecPlacementPolicy,
NodeManager nodeManager) {
sizeEnteringNode = new HashMap<>();
this.containerManager = containerManager;
this.placementPolicy = placementPolicy;
+ this.ecPlacementPolicy = ecPlacementPolicy;
Review Comment:
I think it might be better to not have different placement policy fields? We need a better way to fetch these placement policies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1165552039
We have a JIRA [HDDS-6940](https://issues.apache.org/jira/browse/HDDS-6940) for excluding EC containers from balancing. Looks like we need further discussions for the EC case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] umamaheswararao commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1193008772
@lokeshj1703 could you please take a look at the latest patch from @JacksonYao287
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1199076818
@JacksonYao287 Thanks for the contribution! @siddhantsangwan Thanks for the review! I have merged the PR to master branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1198997097
thanks @lokeshj1703 for the question.
we need to support move EC at both containerBalancer side and RM side. this patch aims to add EC support at containerBalancer side. as i said , all the EC container process will not be forwarded to legacyRM , so we do not support move EC at RM side until (https://issues.apache.org/jira/browse/HDDS-6572) is done.
@siddhantsangwan has a patch [HDDS-6940](https://issues.apache.org/jira/browse/HDDS-6940) to excluding EC containers from balancing. so actually, EC move does not work for now.
after , HDDS-6572 is done and this patch is merged, HDDS-6940 will be reverted , then EC move will really work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1196628387
@JacksonYao287 Thanks for updating the PR! The changes look good. I was thinking about balancing EC containers and was wondering if other changes would also be required in subsequent jiras. Few questions.
- Do we want to balance EC containers using average EC utilization of DNs as well?
- Does RM handle move for EC containers today in the move API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1149617723
thanks @lokeshj1703 and @siddhantsangwan for the review, i have updated this patch , please take a look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1197988971
> I think there is a new RM being added as part of EC. Please refer https://issues.apache.org/jira/browse/HDDS-6589.
>
> I think the handling of over and under replicated containers is different in EC. Since move relies on the RM handling of over-replicated containers, logic should be affected for move in case of EC containers. Please correct me if I am wrong.
we don`t want to implement EC over and under-replicated handling logic in RM directly since it is too complex. so we convert old RM to legacyReplicationManager and create a new RM. then Ratis container will be forwarded by the newly created one to legacyReplicationManager, so that we can keep the current Ratis container process flow. for EC container, it will be processed in the newly created RM by the newly implmented over and under replicated handlers. so , in fact , we have two separate process flow for Ratis and EC container.
also, any call to replicationManager#move will be forwarded to legacyReplicationManager#Move, so the current move api will not be affected. move api should be a independent functionality which is not aware of the container type, and i have a jira(https://issues.apache.org/jira/browse/HDDS-6572) to split move scheduler from legacy RM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on PR #3455:
URL: https://github.com/apache/ozone/pull/3455#issuecomment-1196930808
thanks @lokeshj1703 for the review!
>Do we want to balance EC containers using average EC utilization of DNs as well?
containers of different type could be exist in the same datanode, so they should be equal for balancing and no need to distinguish them on utilization. the only difference is the target datanode is selected according to different placement policy, which is what this patch does
>Does RM handle move for EC containers today in the move API?
yes , move api is also not aware of the container type. it just send a replication command to target datanode and then send a deletion command to source datanode. no changes need to be carried out on move api.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] lokeshj1703 commented on a diff in pull request #3455: HDDS-6533. support balancing EC container
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on code in PR #3455:
URL: https://github.com/apache/ozone/pull/3455#discussion_r929586692
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/AbstractFindTargetGreedy.java:
##########
@@ -45,7 +44,7 @@
public abstract class AbstractFindTargetGreedy implements FindTargetStrategy {
private Logger logger;
private ContainerManager containerManager;
- private PlacementPolicy placementPolicy;
+ private PlacementPolicyValidateProxy placementPolicyValidateProxy;
Review Comment:
Can we create an instance of it inside SCM? I think it can be used by balancer and other components as well.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -607,8 +607,7 @@ private void initializeSystemManagers(OzoneConfiguration conf,
long term = SCMHAUtils.isSCMHAEnabled(conf) ? 0 : SCMContext.INVALID_TERM;
// non-leader of term 0, in safe mode, preCheck not completed.
scmContext = new SCMContext.Builder()
- .setLeader(false)
- .setTerm(term)
+ .setLeader(false).setTerm(term)
Review Comment:
unintended change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org