sodonnel commented on a change in pull request #3085:
URL: https://github.com/apache/ozone/pull/3085#discussion_r808982225
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java
##########
@@ -123,22 +121,30 @@ public void onMessage(final ContainerReportFromDatanode
reportFromDatanode,
final Set<ContainerID> containersInSCM =
nodeManager.getContainers(datanodeDetails);
- final Set<ContainerID> containersInDn = replicas.parallelStream()
- .map(ContainerReplicaProto::getContainerID)
- .map(ContainerID::valueOf).collect(Collectors.toSet());
-
- final Set<ContainerID> missingReplicas = new
HashSet<>(containersInSCM);
- missingReplicas.removeAll(containersInDn);
-
- processContainerReplicas(datanodeDetails, replicas, publisher);
- processMissingReplicas(datanodeDetails, missingReplicas);
-
- /*
- * Update the latest set of containers for this datanode in
- * NodeManager
- */
- nodeManager.setContainers(datanodeDetails, containersInDn);
-
+ for (ContainerReplicaProto replica : replicas) {
+ ContainerID cid = ContainerID.valueOf(replica.getContainerID());
+ ContainerInfo container = null;
+ try {
+ // We get the container using the ContainerID object we obtained
+ // from protobuf. However we don't want to store that object if
+ // there is already an instance for the same ContainerID we can
+ // reuse.
+ container = containerManager.getContainer(cid);
+ cid = container.containerID();
+ } catch (ContainerNotFoundException e) {
+ // Ignore this for now. It will be handle later with a null check.
+ }
+
+ boolean wasInSCM = containersInSCM.remove(cid);
+ if (!wasInSCM) {
Review comment:
The logic before this change, always added the ContainerID to the
nodemanager map, even if the container is not present in SCM. It is
questionable on whether it should do that or not - it is hard to think of a
reason why it is useful, but there might be one. As I am trying to improve the
performance here, I would like to keep the logic the same so we don't introduce
some unexpected side effect.
Looking at my new logic, we first set the ContainerID to the value obtained
from the protobuf report. Then I update it to the value stored in the
ContainerInfo if it exists, otherwise just use the one from proto. So this
should work fine.
--
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]
