[GitHub] [ozone] sodonnel commented on a change in pull request #2963: HDDS-5267 Merge ICR and FCR for containers if part of the same HB

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

sodonnel commented on a change in pull request #2963:
URL: https://github.com/apache/ozone/pull/2963#discussion_r812129713



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
##########
@@ -235,18 +235,20 @@ public void listContainer(long startContainerId, long count,
   public ContainerReportsProto getContainerReport() throws IOException {
     LOG.debug("Starting container report iteration.");
 
+    ContainerReportsProto.Builder crBuilder =
+        ContainerReportsProto.newBuilder();
     // No need for locking since containerMap is a ConcurrentSkipListMap
     // And we can never get the exact state since close might happen
     // after we iterate a point.
     List<Container<?>> containers = new ArrayList<>(containerMap.values());
-
-    ContainerReportsProto.Builder crBuilder =
-        ContainerReportsProto.newBuilder();
-
-    for (Container<?> container: containers) {
-      crBuilder.addReports(container.getContainerReport());
+    // Incremental Container reports can read stale container information

Review comment:
       Is there a potential race condition on line 243 where the containers array list is formed? The map is concurrent, so we probably cannot get a concurrentModification error, but could we get a snapshot of the containers here and meanwhile a new container is added and ICR queued that is not in our snapshot and hence never gets added to the FCR?
   
   I think we need to synchronize around the forming of this list.
   
   I also wonder why we need to form the new list? I guess we can release any lock faster if we are not forming the protobuf under the lock, but then if we synchronize here to block ICRs, I think we are effectively blocks new container creation anyway. Maybe we can avoid the new array list?
   
   Need to think about this carefully.




-- 
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: issues-unsubscribe@ozone.apache.org

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



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