jojochuang commented on code in PR #11054:
URL: https://github.com/apache/ozone/pull/11054#discussion_r3809314093
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -536,17 +601,35 @@ public void sendThrottledReplicationCommand(ContainerInfo
containerInfo,
public void sendThrottledReconstructionCommand(ContainerInfo containerInfo,
ReconstructECContainersCommand command)
throws CommandTargetOverloadedException, NotLeaderException {
- List<DatanodeDetails> targets = command.getTargetDatanodes();
- List<Pair<Integer, DatanodeDetails>> targetWithCmds =
- getAvailableDatanodesForReplication(targets);
- if (targetWithCmds.isEmpty()) {
+ if (!tryReserveReconstructionSlot()) {
metrics.incrECReconstructionCmdsDeferredTotal();
- throw new CommandTargetOverloadedException("No target with capacity " +
- "available for reconstruction of " + containerInfo.getContainerID());
+ throw new CommandTargetOverloadedException(
+ "Global reconstruction limit (" + getReconstructionInFlightLimit()
+ + ") reached for container " + containerInfo.getContainerID());
+ }
+ final long cmdId = command.getId();
+ final int fragmentCount = command.getMissingContainerIndexes().size();
Review Comment:
**Medium — slot leak on empty indexes**
`fragmentCount == 0` reserves a global slot and registers the cmdId, but
`adjustPendingOpsAndMetrics` schedules zero pending ADD ops, so `opCompleted()`
never fires and the slot is never released.
Suggest rejecting empty `missingContainerIndexes` and calling
`releaseReconstructionSlot()` before throw.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -1059,6 +1142,22 @@ ReplicationQueue getQueue() {
@Override
public void opCompleted(ContainerReplicaOp op, ContainerID containerID,
boolean timedOut) {
Review Comment:
**High — slot held until event timeout on DN failure**
Decrement only happens here (or on ADD expiry via `removeExpiredEntries`).
DN failure reports still do not clear pending ADD ops (HDDS-15327), so a failed
reconstruction keeps its global slot until SCM event timeout (~12 min).
With `reconstruction.global.limit > 0`, this can exhaust the cluster cap
during decommission. Consider tying slot release to pending-op removal or
landing HDDS-15327 first.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -536,17 +601,35 @@ public void sendThrottledReplicationCommand(ContainerInfo
containerInfo,
public void sendThrottledReconstructionCommand(ContainerInfo containerInfo,
ReconstructECContainersCommand command)
throws CommandTargetOverloadedException, NotLeaderException {
- List<DatanodeDetails> targets = command.getTargetDatanodes();
- List<Pair<Integer, DatanodeDetails>> targetWithCmds =
- getAvailableDatanodesForReplication(targets);
- if (targetWithCmds.isEmpty()) {
+ if (!tryReserveReconstructionSlot()) {
metrics.incrECReconstructionCmdsDeferredTotal();
Review Comment:
**Low — observability**
Both global-limit rejection (here) and per-DN overload rejection (line ~619)
increment `ec_reconstruction_cmds_deferred_total`. Operators cannot distinguish
cluster-cap deferrals from datanode saturation. A dedicated global-limit
counter would help (HDDS-15075).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -1266,6 +1365,39 @@ public static class ReplicationManagerConfiguration
)
private int containerSampleLimit = 100;
+ @Config(key =
"hdds.scm.replication.decommission.ec.reconstruction.enabled",
+ type = ConfigType.BOOLEAN,
+ defaultValue = "false",
+ reconfigurable = true,
+ tags = { SCM },
+ description = "If true, SCM will switch from 1-1 replication to " +
+ "multi-source reconstruction for EC containers on decommissioning
" +
+ "nodes when the node's load exceeds the threshold."
+ )
+ private boolean ecDecommissionReconstructionEnabled = false;
+
+ @Config(key =
"hdds.scm.replication.decommission.ec.reconstruction.load.factor",
+ type = ConfigType.DOUBLE,
+ defaultValue = "0.9",
+ reconfigurable = true,
+ tags = { SCM },
+ description = "The threshold factor (between 0 and 1) of a node's " +
+ "replication limit at which SCM switches to reconstruction for " +
+ "EC decommission. Default is 0.9."
+ )
+ private double ecDecommissionReconstructionLoadFactor = 0.9;
+
+ @Config(key = "hdds.scm.replication.reconstruction.global.limit",
+ type = ConfigType.INT,
+ defaultValue = "0",
+ reconfigurable = true,
+ tags = { SCM },
+ description = "A cluster-wide limit to restrict the total number of " +
Review Comment:
**Low — runtime reconfig note**
If `reconstruction.global.limit` is lowered while inflight > new limit,
existing commands continue and new sends are blocked until completion. Worth
documenting in the config description.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestReplicationManager.java:
##########
@@ -1739,4 +1745,312 @@ private void mockReplicationCommandCounts(
});
}
+ @Test
+ public void testReconstructionGlobalLimitDisabledByDefault()
Review Comment:
**Medium — missing test coverage**
Consider adding tests for:
- Expired reconstruction ADD op (`opCompleted(..., timedOut=true)`)
decrements `inflightReconstructionCount`
- (After HDDS-15327) DN failure report releases the global slot promptly
These failure/timeout paths are the most likely to cause production stalls
when the global cap is enabled.
--
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]
