swamirishi commented on PR #7402:
URL: https://github.com/apache/ozone/pull/7402#issuecomment-2483000946
> Thanks for reducing the changed files. Its down to 47 now from 97, which
is a big improvement.
>
> I still think its confusing that we have two structures (missingContainers
map and ContainerIDsTable) to track what is going on.
>
> If we want to block creating a container that has been created before, can
we just check for existence in the ContainerIDsTable? Right now we check the
missingTable and then add an entry to the containerIDTable.
MissingContainerSet is always in memory. We add it to the containerIdTable
and remove the entry from the missingContainerSet.
> If we remove a container, there are two boolean parameters - markMissing
and removeFromDB - is it ever valid to pass "true, true" so it marks it missing
and removes it from the DB?
removeFromDB is only false when the physical container directory is not
deleted from the node. So true, true should not be possible.
**false, false** : This happens on finding a duplicate container on startup
where the container info in memory gets swapped by removing and adding the
updated container info.
**true, false** : This happens on volume failure and we have to mark it as
missing. This is a plain inmemory operation where in we remove the container
from containerMap and add the containerId to the missingContainerSet.
**false, true** : This is a regular delete flow which happens when SCM sends
a delete container command. This will only happen after deleting the entire
container data from the disk.
**true, true** : This is an invalid case, this doesn't happen anywhere.
> Provided we always add to the containerIDsTable on new containerCreation,
then we should be able to check for previous creation from it too.
Since we have the containerInfo in the containerMap in memory already.
There is no point in going to the rocksdb.
> It feels like we can remove the container for a failed volume we don't
touch the containerIDs table. Something that tries to recreate it will get
blocked by the existence chance against the table.
>
> Or, we remove it "safely" via replication, balancer and we remove it from
the DB.
On replication we always have the overwrite flag
**overwriteMissingContainers** set to true.
> If we just use the containerID table, then you don't need to iterate it at
startup and checkoff all the containers. You also don't need to hold anything
in memory as the RocksDB will cache things itself. However we may need to
populate the table from all existing containers on disk.
This is not an expensive operation. We anyways keep the entire containerMap
in memory. There is no point in adding an overhead of rocksdb get, when we have
the entire data in memory.
>
> I also suspect the code would be more understandable if we didn't overload
add/removeContainer in ContainerSet with the booleans, and instead create a new
method `add???` and `remove???` which are named more inline with what they are
doing, eg remove, removeSafely, removeUnexpectedly, or something along those
lines.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
