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


##########
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:
   Thanks @ashishkumar50 for the questions.
   
   One clarification on Vol4: if a DN volume path is not explicitly tagged with 
a storage type, DN/SCM still do not keep it as an untyped volume in the 
placement model.
   
   The path is:
   1. DN parses the configured volume location and gets a `StorageType` ([Ozone 
call 
site](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java#L155-L161)).
   2. Hadoop `StorageLocation.parse()` defaults an untagged location to 
`StorageType.DEFAULT` 
([code](https://github.com/apache/hadoop/blob/rel/release-3.4.3/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/StorageLocation.java#L129-L144)).
   3. `StorageType.DEFAULT` is `DISK` 
([StorageType.DEFAULT](https://github.com/apache/hadoop/blob/rel/release-3.4.3/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/StorageType.java#L47)).
   4. Ozone reports that volume storage type back to SCM 
([code](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java#L500-L505)).
   
   So the example is closer to:
   
   Vol1: SSD  
   Vol2: DISK  
   Vol3: DISK  
   Vol4: DEFAULT(DISK), if it was not explicitly tagged
   
   For case 1:
   - if the request has `StorageType.SSD`, DN will only choose Vol1.
   - if the request has `StorageType.DISK`, DN can choose Vol2, Vol3, or Vol4.
   
   For case 2, if the request has no storage type (`storageType == null`), DN 
does not filter by storage type. The `VolumeChoosingPolicy` API defines null as 
allowing the policy to choose from any volume 
([interface](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/VolumeChoosingPolicy.java#L40-L46)),
 and the implementation keeps the full candidate list when `storageType` is 
null before calling `chooseVolumeInternal(...)` 
([code](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)).
   
   
   
   
   In addition, `unknownBucket` should be uncommon in the new allocation path, 
because SCM defaults a missing storage tier to the configured default tier 
before allocation 
([code](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java#L245-L248),
 [default 
config](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/common/src/main/resources/ozone-default.xml#L4447-L4458)).
   
   It is mainly for backward compatibility. For example, when RM repairs or 
reconstructs a legacy container whose `ContainerInfo` has no `storageTier`, 
`getStorageType(containerInfo)` returns `null` and that value is passed into 
the ADD pending op ([call 
site](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java#L697-L701),
 
[helper](https://github.com/apache/ozone/blob/dfd9b9b2ba1d3abd46ea1dc16bce56d17c7673ea/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java#L721-L724)).



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

Reply via email to