rakeshadr commented on code in PR #10497:
URL: https://github.com/apache/ozone/pull/10497#discussion_r3413863102
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:
##########
@@ -298,29 +296,21 @@ public List<DatanodeDetails>
filterNodesWithSpace(List<DatanodeDetails> nodes,
*/
public static boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
Review Comment:
update javadoc with the args
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:
##########
@@ -298,29 +296,21 @@ public List<DatanodeDetails>
filterNodesWithSpace(List<DatanodeDetails> nodes,
*/
public static boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long metadataSizeRequired,
- long dataSizeRequired) {
+ long dataSizeRequired,
+ NodeManager nodeManager) {
Preconditions.checkArgument(datanodeDetails instanceof DatanodeInfo);
- boolean enoughForData = false;
boolean enoughForMeta = false;
DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ // Data-space check: use PendingContainerTracker slot availability.
+ // This accounts for both current disk usage and in-flight allocations.
if (dataSizeRequired > 0) {
Review Comment:
`#hasAvailableSpace()` function checks for a full 5 GB `maxContainerSize`
slot, so a dn with 4.9 GB free gets rejected.
Remove `dataSizeRequired` from the signature entirely and rename the method
to make the slot-based contract clear — since the value is no longer used and
tracker's approach is always maxContainerSize.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -1468,8 +1473,9 @@ static class NonWritableNodeFilter implements
Predicate<DatanodeInfo> {
private final long blockSize;
private final long minRatisVolumeSizeBytes;
private final long containerSize;
+ private final NodeManager nodeManager;
- NonWritableNodeFilter(ConfigurationSource conf) {
+ NonWritableNodeFilter(ConfigurationSource conf, NodeManager nodeManager) {
Review Comment:
`this` passed before constructor returns
Can't we pass `'tracker'` instead of `'nodeManager'` ?
`NonWritableNodeFilter(ConfigurationSource conf, PendingContainerTracker
tracker) {`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java:
##########
Review Comment:
It seems all the pending tracker logs are `debug` level. Nothing is
`LOG.warn` or `LOG.info`. In production, none of this is visible without
enabling `DEBUG` logging for PendingContainerTracker.
```
if (dropped > 0) {
LOG.warn("PendingContainerTracker: force-dropped {} unconfirmed pending
containers "
+ "on DN {} after {}ms (2x rollInterval). "
+ "Container reports may have been lost.",
dropped, datanodeID, elapsed);
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java:
##########
@@ -208,12 +208,38 @@ public boolean checkSpaceAndRecordAllocation(DatanodeInfo
datanodeInfo, Containe
}
/**
- * Remove a pending container allocation from a specific DataNode.
- * Removes from both current and previous windows.
- * Called when container is confirmed.
+ * Returns true if the given datanode has at least one allocatable container
slot
+ * available, accounting for pending in-flight allocations.
*
- * @param containerID The container to remove from pending
+ * Uses the same slot-counting logic as {@link
TwoWindowBucket#checkSpaceAndAdd}
+ *
+ * <p>This method does not consume a slot — it is a read-only check intended
+ * for the placement policy
+ *
+ * @param datanodeInfo the datanode to check
+ * @return true if at least one container slot is available
*/
+ public boolean hasAvailableSpace(DatanodeInfo datanodeInfo) {
+ Objects.requireNonNull(datanodeInfo, "datanodeInfo == null");
+ List<StorageReportProto> storageReports = datanodeInfo.getStorageReports();
+ if (storageReports.isEmpty()) {
+ return false;
+ }
+ TwoWindowBucket bucket = datanodeInfo.getPendingContainerAllocations();
+ bucket.rollIfNeeded();
+ final int pendingCount = bucket.getCount();
+ long allocatableCount = 0;
+ for (StorageReportProto report : storageReports) {
+ allocatableCount += VolumeUsage.getUsableSpace(report) /
maxContainerSize;
+ if (allocatableCount > pendingCount) {
+ return true;
+ }
+ }
+ LOG.debug("Datanode {} has no available container slots. Pending: {},
Allocatable: {}",
+ datanodeInfo.getID(), pendingCount, allocatableCount);
+ return false;
Review Comment:
Since we are skipping the node, we need to capture metrics.
```
if (metrics != null) {
metrics.incNumSkippedFullNodeContainerAllocation();
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java:
##########
@@ -208,12 +208,38 @@ public boolean checkSpaceAndRecordAllocation(DatanodeInfo
datanodeInfo, Containe
}
/**
- * Remove a pending container allocation from a specific DataNode.
- * Removes from both current and previous windows.
- * Called when container is confirmed.
+ * Returns true if the given datanode has at least one allocatable container
slot
+ * available, accounting for pending in-flight allocations.
*
- * @param containerID The container to remove from pending
+ * Uses the same slot-counting logic as {@link
TwoWindowBucket#checkSpaceAndAdd}
+ *
+ * <p>This method does not consume a slot — it is a read-only check intended
+ * for the placement policy
+ *
+ * @param datanodeInfo the datanode to check
+ * @return true if at least one container slot is available
*/
+ public boolean hasAvailableSpace(DatanodeInfo datanodeInfo) {
+ Objects.requireNonNull(datanodeInfo, "datanodeInfo == null");
+ List<StorageReportProto> storageReports = datanodeInfo.getStorageReports();
+ if (storageReports.isEmpty()) {
+ return false;
+ }
+ TwoWindowBucket bucket = datanodeInfo.getPendingContainerAllocations();
+ bucket.rollIfNeeded();
+ final int pendingCount = bucket.getCount();
+ long allocatableCount = 0;
+ for (StorageReportProto report : storageReports) {
+ allocatableCount += VolumeUsage.getUsableSpace(report) /
maxContainerSize;
+ if (allocatableCount > pendingCount) {
+ return true;
+ }
+ }
+ LOG.debug("Datanode {} has no available container slots. Pending: {},
Allocatable: {}",
+ datanodeInfo.getID(), pendingCount, allocatableCount);
+ return false;
+ }
+
public void removePendingAllocation(TwoWindowBucket bucket, ContainerID
containerID) {
Review Comment:
Javadoc missing, please add
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java:
##########
@@ -208,12 +208,38 @@ public boolean checkSpaceAndRecordAllocation(DatanodeInfo
datanodeInfo, Containe
}
/**
- * Remove a pending container allocation from a specific DataNode.
- * Removes from both current and previous windows.
- * Called when container is confirmed.
+ * Returns true if the given datanode has at least one allocatable container
slot
+ * available, accounting for pending in-flight allocations.
*
- * @param containerID The container to remove from pending
+ * Uses the same slot-counting logic as {@link
TwoWindowBucket#checkSpaceAndAdd}
+ *
+ * <p>This method does not consume a slot — it is a read-only check intended
+ * for the placement policy
+ *
+ * @param datanodeInfo the datanode to check
+ * @return true if at least one container slot is available
*/
+ public boolean hasAvailableSpace(DatanodeInfo datanodeInfo) {
+ Objects.requireNonNull(datanodeInfo, "datanodeInfo == null");
+ List<StorageReportProto> storageReports = datanodeInfo.getStorageReports();
+ if (storageReports.isEmpty()) {
+ return false;
+ }
+ TwoWindowBucket bucket = datanodeInfo.getPendingContainerAllocations();
+ bucket.rollIfNeeded();
+ final int pendingCount = bucket.getCount();
+ long allocatableCount = 0;
+ for (StorageReportProto report : storageReports) {
+ allocatableCount += VolumeUsage.getUsableSpace(report) /
maxContainerSize;
Review Comment:
Can we make the ceiling explicitly to 0:
`Math.max(0L, VolumeUsage.getUsableSpace(report)) / maxContainerSize;`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java:
##########
@@ -208,12 +208,38 @@ public boolean checkSpaceAndRecordAllocation(DatanodeInfo
datanodeInfo, Containe
}
/**
- * Remove a pending container allocation from a specific DataNode.
- * Removes from both current and previous windows.
- * Called when container is confirmed.
+ * Returns true if the given datanode has at least one allocatable container
slot
+ * available, accounting for pending in-flight allocations.
*
- * @param containerID The container to remove from pending
+ * Uses the same slot-counting logic as {@link
TwoWindowBucket#checkSpaceAndAdd}
+ *
+ * <p>This method does not consume a slot — it is a read-only check intended
+ * for the placement policy
+ *
+ * @param datanodeInfo the datanode to check
+ * @return true if at least one container slot is available
*/
+ public boolean hasAvailableSpace(DatanodeInfo datanodeInfo) {
+ Objects.requireNonNull(datanodeInfo, "datanodeInfo == null");
+ List<StorageReportProto> storageReports = datanodeInfo.getStorageReports();
+ if (storageReports.isEmpty()) {
+ return false;
+ }
+ TwoWindowBucket bucket = datanodeInfo.getPendingContainerAllocations();
+ bucket.rollIfNeeded();
+ final int pendingCount = bucket.getCount();
+ long allocatableCount = 0;
+ for (StorageReportProto report : storageReports) {
+ allocatableCount += VolumeUsage.getUsableSpace(report) /
maxContainerSize;
+ if (allocatableCount > pendingCount) {
+ return true;
+ }
+ }
+ LOG.debug("Datanode {} has no available container slots. Pending: {},
Allocatable: {}",
Review Comment:
Suggestion: Please check whether we need to add a safety warn check?
// Warn if pending count is physically impossible given current disk
sizes.
// This indicates container reports are significantly delayed or the disk
// shrank faster than the heartbeat cycle. Entries age out after 2 *
rollInterval.
if (pendingCount > allocatableTotal * 2) {
LOG.warn("Datanode {} has unusually high pending count: pending={},
allocatable={}. "
+ "Container reports may be delayed. Slots will age out in
~{}min.",
datanodeInfo.getID(), pendingCount, allocatableTotal,
(2 * rollIntervalMs) / 60_000);
}
Example case:
How the numbers flow through with the example from before
```
Volume 1: usable=44GB → 44/5 = 8 slots
Volume 2: usable=44GB → 8 slots
Volume 3: usable=44GB → 8 slots
allocatableTotal = 24
Normal: pendingCount=5, 5 > 48? NO warn. 24 > 5? → return true
Burst: pendingCount=20, 20 > 48? NO warn. 24 > 20? → return true (4 free)
Full: pendingCount=24, 24 > 48? NO warn. 24 > 24? → return false,
metric++
Delayed: pendingCount=55, 55 > 48? YES → LOG.warn "age out in ~10min"
24 > 55? → return false, metric++
```
--
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]
