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

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

sodonnel edited a comment on pull request #1338:
URL: https://github.com/apache/hadoop-ozone/pull/1338#issuecomment-676472849


   Thanks for working on this @ChenSammi.
   
   I wonder if it would be simpler to remove empty containers as part of Container Report processing? In `AbstractContainerReportHandler#updateContainerState`, we could check the size and number of keys of the reported containers in the CLOSED branch of the switch statement, and then take action to delete an empty container there? I have a feeling it might be simpler, but I am not sure. The disadvantage of doing it in the Container Report Processing, is that we are dealing with only a single replica at that stage. However if the container is CLOSED in SCM, and a report says it is empty then we should be good to simply remove the container from SCM and issue the delete container command when processing the container report.
   
   In looking at how this all works, I noticed there is a bug in the existing method AbstractContainerReportHandler#updateContainerStats, which might stop your change working:
   
   ```
     private void updateContainerStats(final ContainerID containerId,
                                       final ContainerReplicaProto replicaProto)
         throws ContainerNotFoundException {
       if (isHealthy(replicaProto::getState)) {
         final ContainerInfo containerInfo = containerManager
             .getContainer(containerId);
   
         if (containerInfo.getSequenceId() <
             replicaProto.getBlockCommitSequenceId()) {
           containerInfo.updateSequenceId(
               replicaProto.getBlockCommitSequenceId());
         }
         if (containerInfo.getUsedBytes() < replicaProto.getUsed()) {
           containerInfo.setUsedBytes(replicaProto.getUsed());
         }
         if (containerInfo.getNumberOfKeys() < replicaProto.getKeyCount()) {
           containerInfo.setNumberOfKeys(replicaProto.getKeyCount());
         }
       }
     }
   ```
   
   This logic assumes the bytes used and keyCount is always increasing on the container. However in this case, where blocks are deleted the keyCount and bytesUsed can decrease. Therefore I don't think the stats will ever get updated in SCM via the container reports when blocks are removed.
   
   I then noticed you recently made a change in ReplicationManager to update these counts here:
   
   https://github.com/apache/hadoop-ozone/pull/1295/files#diff-218132dfe72e6e39b9eaaf59737adcf2R780
   
   I think the ReplicationManager part of that change should be reverted and it handled via the ContainerReports by fixing the bug I mentioned above. The only reason we need the Replication Manager change is to work around the bug above. If it was fixed, we don't need that logic in RM.
   
   If we decide to keep your changes inside replicationManager (for this change), then we would need a few more tests added to TestReplicationManager to cover the new scenarios.
   
   I also have a question - when the replicas are all deleted, and we call:
   
   ```
   containerManager.updateContainerState(container.containerID(),
             HddsProtos.LifeCycleEvent.CLEANUP);
   ```
   
   Does this somehow cause the container to be removed from SCM memory and the persistent store?


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