HDDS-7384. EC: ReplicationManager - implement deleting container handler#3881
HDDS-7384. EC: ReplicationManager - implement deleting container handler#3881siddhantsangwan merged 9 commits intoapache:masterfrom
Conversation
...n/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java
Outdated
Show resolved
Hide resolved
|
This change looks much better now. There is just a little more work to do to remove the ContainerManager dependency from the TestEmptyContainerHandler and then I think it is good. It would be good for @siddhantsangwan to have a look too, as he has been working in this area too recently. |
|
@JacksonYao287 Thanks for working on this. I'll review soon. |
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
LGTM - thanks for taking the changes onboard. Lets let @siddhantsangwan have a look before we commit please.
siddhantsangwan
left a comment
siddhantsangwan
left a comment
There was a problem hiding this comment.
The changes look good. I just have a few minor comments. Also, do we need another test for confirming the handler returns true for DELETED containers?
...n/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java
Outdated
Show resolved
Hide resolved
|
@siddhantsangwan thanks for the review! i have updated this patch , please take a look |
|
@JacksonYao287 Changes look good. The test failure is in Replication Manager. Can you please check? |
this test successes locally on my laptop, i am not sure why it fails here. i retrigger CI to see whether it can be reproduced! |
|
@siddhantsangwan PTAL! |
| /** | ||
| * If a container is in Deleting state and no replica exists, | ||
| * change the state of the container to DELETED. | ||
| */ |
There was a problem hiding this comment.
hmm~ just for myself learning~~ is it possible something goes wrong in Deleting state, then the container is not deleted successfully?
There was a problem hiding this comment.
for now , a container is deleted means its state is changed to DELETED. it is not be removed from scm , so scm still has the reference of this container.
#3360 this patch is trying to remove the reference from scm when its state is DELETED.
I am not sure whether i have answered your question. if not , please let me know!
4 participants
What changes were proposed in this pull request?
implement deleting container handler
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7384
How was this patch tested?
unit test