[GitHub] [ozone] adoroszlai commented on a diff in pull request #4955: HDDS-8897. Avoid synchronization on WritableECContainerProvider

["adoroszlai (via GitHub)" <gi...@apache.org>]

adoroszlai commented on code in PR #4955:
URL: https://github.com/apache/ozone/pull/4955#discussion_r1238771240


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -150,21 +153,70 @@ public ContainerInfo getContainer(final long size,
         }
       }
     }
+
     // If we get here, all the pipelines we tried were no good. So try to
     // allocate a new one.
+    container = allocateContainerIfWithinLimit(
+        maximumPipelines, openPipelineCount, true,
+        repConfig, size, owner, excludeList);
+
+    if (container != null) {
+      return container;
+    }
+
+    String msg = "Unable to allocate a pipeline for " + repConfig + ":"
+        + " the maximum of " + maximumPipelines + " has been reached";
+    if (openPipelineCount > 0) {
+      msg += ", and none of the " + openPipelineCount
+          + " existing ones are suitable";
+    }
+
+    throw new IOException(msg);
+  }
+
+  @Nullable
+  private ContainerInfo allocateContainerIfWithinLimit(
+      int max, int current, boolean finalAttempt,
+      ECReplicationConfig repConfig, long size, String owner,
+      ExcludeList excludeList) throws IOException, TimeoutException {
+
+    final String msg = "Unable to allocate a container for {} as {} existing "
+        + "containers and {} pending allocations have reached the limit of {}";
+
+    final int pending = pendingAllocations.getAndIncrement();
     try {
-      synchronized (this) {
-        if (openPipelineCount < maximumPipelines) {
-          return allocateContainer(repConfig, size, owner, excludeList);
+      if (current + pending < max) {
+        ContainerInfo containerInfo =
+            allocateContainer(repConfig, size, owner, excludeList);
+        allocation.signal();

Review Comment:
   You are right.  It would be woken up by the next future allocation, though.  We may change to `await(time)` to limit that period.



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