Re: [PR] HDDS-16045. Make PendingContainerTracker StorageType aware [ozone]

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

F64116045 commented on code in PR #10928:
URL: https://github.com/apache/ozone/pull/10928#discussion_r3899595483


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java:
##########
@@ -143,14 +157,98 @@ synchronized boolean remove(ContainerID containerID) {
     synchronized int getCount() {
       return currentWindow.size() + previousWindow.size();
     }
+  }
+
+  /**
+   * Pending container allocations for one datanode, grouped by storage type.
+   */
+  public static class PendingContainerAllocations {
+    private final Map<StorageType, TwoWindowBucket> typedBuckets =
+        new EnumMap<>(StorageType.class);
+    private final TwoWindowBucket unknownBucket;
+    private final long rollIntervalMs;
+    private final DatanodeID datanodeID;
+
+    PendingContainerAllocations(DatanodeID id, long rollIntervalMs) {
+      this.datanodeID = id;
+      this.rollIntervalMs = rollIntervalMs;
+      this.unknownBucket = new TwoWindowBucket(id, rollIntervalMs);
+    }
+
+    synchronized void rollIfNeeded() {
+      unknownBucket.rollIfNeeded();
+      typedBuckets.values().forEach(TwoWindowBucket::rollIfNeeded);
+    }
+
+    synchronized boolean contains(ContainerID containerID) {
+      return unknownBucket.contains(containerID)
+          || typedBuckets.values().stream()
+          .anyMatch(bucket -> bucket.contains(containerID));
+    }
+
+    /**
+     * Count pending containers of the given storage type.
+     * Unknown storage type entries are counted for typed checks because they
+     * may occupy the requested storage type.
+     */
+    synchronized int getCount(StorageType storageType) {
+      if (checksAllStorageTypes(storageType)) {
+        return getCount();
+      }
+      TwoWindowBucket bucket = typedBuckets.get(storageType);
+      return unknownBucket.getCount() + (bucket != null ? bucket.getCount() : 0);

Review Comment:
   This came from the earlier non-tier case discussed [here](https://github.com/apache/ozone/pull/10928#discussion_r3703620354).
   
   
   The idea was that a null-typed pending allocation may still occupy a real DISK or SSD volume. `unknownBucket` is not a separate physical storage pool. 
   So when `storageType` is null, the DN volume policy [keeps all volumes as candidates](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/AbstractStorageTypeChoosingPolicy.java#L36-L42) instead of filtering by type.
   
   
   Counting it in typed checks was meant to avoid overestimating available space. 
   
   But I see your point that this is conservative strategy. I think the safer behavior is to avoid over-allocation here, but happy to align if we want to optimize for better tier utilization instead.
   
   cc @devmadhuu 



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