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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
