[GitHub] [hadoop-ozone] sodonnel commented on pull request #1338: HDDS-4023. Delete closed container after all blocks have been deleted.

[GitBox <gi...@apache.org>]

sodonnel commented on pull request #1338:
URL: https://github.com/apache/hadoop-ozone/pull/1338#issuecomment-688774840


   As I said before, I have a feeling that it *might* be better to have a separate service for deleting empty containers, and it would perhaps naturally be part of a "merge small containers" service, which we will need to create eventually. However, the changes to the replication manager here are not too big, and you could argue the current solution fits here OK. Therefore I am OK with keeping this logic in the replication manager. However when it comes to merging small containers in the future, I believe we should create a separate service and not clutter RM with that new feature too.
   
   The patch looks mostly good to me, there are just a couple of areas we need to consider.
   
   1. What do we do with the containers in DELETED state, and with no replicas? I don't think we should keep them in SCM forever, but we are also worried about removing them. However that said, if the replicas have all been deleted, what use is there in keeping the container in SCM?
   
   2. Consider if a datanode was dead for some time, and is then restarted. It will start reporting a replica for the DELETED container and with the current logic I don't think we will clean it up as `isContainerEmpty(...)` will always return false due to it checking for only CLOSED containers. I feel that this should be handled in the ContainerReportHandler code. If a replica is reported for a container which is DELETED in SCM, then we should instruct the datanode to delete the replica. What do you think? The parameter hdds.scm.unknown-container.action currently controls what to do if a DN reports a containers which SCM does not know about. The default is to log a warning, but for a DELETED container, I think we should just delete the replica.
   
   3. There is another corner case, where the dead node may have a container which is out of sync with the others, as it was dead when the blocks should have been removed. Therefore we had 3 empty container replicas, plus one non-empty dead one. The 3 empty replicas are removed, and the container moved to "DELETED". Then the dead node starts reporting a non-empty replica for the DELETED container. This is actually a wider problem, in that I believe there are scenarios where container replicas can get out of sync with one another and there is nothing to reconcile them and fix it.
   
   4. In ContainerSafeModeRule, I think we need to consider DELETING and DELETED. If there are a lot of DELETED containers in the cluster, then they will never get replicas and the safemode rule will never get met. I think this could be as simple as excluding DELETING and DELETED when loading the containerMap (right now OPEN and CLOSING are excluded) so we don't wait for replicas on these containers.
   
   5. In `ReplicationManager.isContainerUnderReplicated(...)`, should we immediately return true if the containerState is DELETING or DELETED?
   
   6. Are there any other places outside of ReplicationManager and ContainerSafeModeRule where we monitor under / over replicated containers I may have missed?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org