HDDS-4928. Support container move in Replication Manager #2349

JacksonYao287 · 2021-06-18T09:08:20Z

What changes were proposed in this pull request?

add a move API in replication manager , and it will be used by container balancer

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4928

How was this patch tested?

add unit test

JacksonYao287 · 2021-06-18T09:10:52Z

@ChenSammi @GlenGeng @lokeshj1703 @siddhantsangwan PATL!

I will add more test case for move in additional commits of this PR

lokeshj1703

Thanks for working on this PR @JacksonYao287 ! The changes look good to me. I have few comments inline.

I was also thinking if we could simplify the logic in RM#handleOverReplicatedContainer. How about we always try to make the container over-replicated so that move can succeed. I think in handleOverReplicatedContainer if src and target are both present and container is over-replicated, delete can be sent for source.
To make sure that container is over-replicated, maybe changes can be made in ContainerReplicaCount so that move target is not included in replica count? This would make sure that finally handleOverReplicatedContainer would have an over-replicated state so that it can just delete source dn.

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

JacksonYao287 · 2021-06-29T02:15:47Z

Thanks @lokeshj1703 very much for this review, i will update this PR according to your comments.

I was also thinking if we could simplify the logic in RM#handleOverReplicatedContainer. How about we always try to make the container over-replicated so that move can succeed. I think in handleOverReplicatedContainer if src and target are both present and container is over-replicated, delete can be sent for source.

this idea seems good, but i have a little question. i think even if src and target are both present and container is over-replicated , deletion can not be definitely sent for source. if the placementPolicy is not met after src is deleted, i think we can not directly send a delete command to src.

lokeshj1703 · 2021-07-05T06:33:42Z

this idea seems good, but i have a little question. i think even if src and target are both present and container is over-replicated , deletion can not be definitely sent for source. if the placementPolicy is not met after src is deleted, i think we can not directly send a delete command to src.

We can use a simple approach initially where if placement policy is not satisfied with replicas + target - src, we fail the move future.

JacksonYao287 · 2021-07-05T13:02:09Z

i have updated the PR , @lokeshj1703 PTAL! if there is no more logic problem , i will add unit test for move

siddhantsangwan · 2021-07-06T05:08:32Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

+        if (isTargetDnInReplicaSet) {
+          if (isSourceDnInReplicaSet) {
+            // if the target is present, and source disappears somehow,
+            // we can consider move is successful.
+            inflightMoveFuture.get(id).complete(MoveResult.COMPLETED);
+            inflightMove.remove(id);
+            inflightMoveFuture.remove(id);
+          } else {
+            // if the container is in inflightMove and target datanode is
+            // included in the replicas, then swap the source datanode to
+            // first of the replica list if exists, so the source datanode
+            // will be first removed if possible.
+            eligibleReplicas.add(0, eligibleReplicas.remove(sourceDnPos));
+          }


Can you please explain this part? If both target and source are present in replicas, then source should be swapped with the first node in replicas list so it can be deleted, right? And move completes if source is absent while target is present. It seems the logic of if and else parts is opposite.

yea, thanks @siddhantsangwan for pointing out this! I explained the logic in the comment, which is just what you said, but i made a mistake in the code. i will fix this , thanks!

lokeshj1703 · 2021-07-09T11:54:45Z

@JacksonYao287 Thanks for updating the PR! The logic looks good to me.

lokeshj1703 · 2021-07-09T12:03:04Z

@JacksonYao287 There is one optimization which we can use for move.
https://github.com/apache/ozone/pull/2349/files#diff-7b38a0477ad3f17c66925825066d4f153b69efbf37288b2c71cd573313771122R1265-R1270

If placement policy was initially not satisfied then deletion for source can be sent provided placement count remains same after deletion. I think it would be a good idea to abstract out logic here
https://github.com/apache/ozone/pull/2349/files#diff-7b38a0477ad3f17c66925825066d4f153b69efbf37288b2c71cd573313771122R1265-R1270
and reuse it for move and over-replication.

JacksonYao287 · 2021-07-12T02:30:38Z

If placement policy was initially not satisfied then deletion for source can be sent provided placement count remains same after deletion. I think it would be a good idea to abstract out logic here and reuse it for move and over-replication.

yea, looks good, i will make this change and add some unit tests for move.

by the way , i think !ps.isPolicySatisfied() before nowPS.actualPlacementCount() == ps.actualPlacementCount() is useless, we can remove it

lokeshj1703

@JacksonYao287 Thanks for updating the PR! The UTs look really good.
I would suggest adding a UT to make sure RM does not delete replica if placement policy is not satisfied.

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

JacksonYao287 · 2021-07-16T01:14:18Z

@lokeshj1703 Thanks for the review

I would suggest adding a UT to make sure RM does not delete replica if placement policy is not satisfied.

i will add this UT

JacksonYao287 · 2021-07-16T01:14:35Z

@ChenSammi @siddhantsangwan PTAL！

lokeshj1703

@JacksonYao287 Thanks for updating the PR! The changes look good to me. +1.
I have a minor comment inline.

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

HDDS-4928. Support container move in Replication Manager

65b03bd

JacksonYao287 force-pushed the HDDS-4928 branch from 74155a0 to 65b03bd Compare June 19, 2021 11:03

Merge remote-tracking branch 'origin/master' into HDDS-4928

be4a07c

lokeshj1703 reviewed Jun 25, 2021

View reviewed changes

update

f5631ec

add check for replicas + target - src

f94432e

JacksonYao287 force-pushed the HDDS-4928 branch from 7bcfdd7 to f94432e Compare July 5, 2021 13:16

siddhantsangwan reviewed Jul 6, 2021

View reviewed changes

Jackson Yao added 4 commits July 6, 2021 14:21

fix a logic error

3fec3d6

add handleMoveIfNeeded

0d62fef

Merge remote-tracking branch 'origin/master' into HDDS-4928

c6eaa5b

fix findbugs

ce29c94

JacksonYao287 requested a review from lokeshj1703 July 7, 2021 13:08

Merge remote-tracking branch 'origin/master' into HDDS-4928

ce6d151

JacksonYao287 force-pushed the HDDS-4928 branch from 6e71059 to f0c016e Compare July 12, 2021 06:29

update

0c5ca39

JacksonYao287 force-pushed the HDDS-4928 branch from f0c016e to 0c5ca39 Compare July 12, 2021 07:16

Jackson Yao added 3 commits July 13, 2021 14:49

add unit test for move

98abb10

triger CI for unit test

7b65963

Merge remote-tracking branch 'origin/master' into HDDS-4928

87ac247

lokeshj1703 reviewed Jul 15, 2021

View reviewed changes

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java Show resolved Hide resolved

add UT and change hanldMove

c08502f

lokeshj1703 approved these changes Jul 16, 2021

View reviewed changes

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java Outdated Show resolved Hide resolved

Jackson Yao added 4 commits July 16, 2021 19:26

update

b6147be

Merge remote-tracking branch 'origin/master' into HDDS-4928

782f327

add wait time for CI

cccca4d

Merge remote-tracking branch 'origin/master' into HDDS-4928

8f5ad22

JacksonYao287 force-pushed the HDDS-4928 branch from 7b6390f to 8f5ad22 Compare July 19, 2021 01:07

ChenSammi merged commit 4ccc128 into apache:master Jul 19, 2021

JacksonYao287 deleted the HDDS-4928 branch July 19, 2021 06:19

adoroszlai mentioned this pull request Jul 21, 2021

HDDS-5459. Fix compile errors in TestReplicationManager #2446

Merged

HDDS-4928. Support container move in Replication Manager #2349

HDDS-4928. Support container move in Replication Manager #2349

Uh oh!

Conversation

JacksonYao287 commented Jun 18, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

JacksonYao287 commented Jun 18, 2021

Uh oh!

lokeshj1703 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacksonYao287 commented Jun 29, 2021

Uh oh!

lokeshj1703 commented Jul 5, 2021

Uh oh!

JacksonYao287 commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siddhantsangwan Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

lokeshj1703 commented Jul 9, 2021

Uh oh!

lokeshj1703 commented Jul 9, 2021

Uh oh!

JacksonYao287 commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lokeshj1703 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JacksonYao287 commented Jul 16, 2021

Uh oh!

JacksonYao287 commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lokeshj1703 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JacksonYao287 commented Jul 5, 2021 •

edited

Loading

JacksonYao287 commented Jul 12, 2021 •

edited

Loading

JacksonYao287 commented Jul 16, 2021 •

edited

Loading