HDDS-15071. [SCM] Add configuration and global EC reconstruction limit - #11054
HDDS-15071. [SCM] Add configuration and global EC reconstruction limit#11054jojochuang wants to merge 9 commits into
Conversation
Set hdds.scm.replication.reconstruction.global.limit default to 0 so cluster-wide EC reconstruction throttling is opt-in and default behavior matches pre-PR Ozone. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I6a0915cf8b181a1a98b08a4f3ede071e4d460e88
… reset. Move global reconstruction throttling from the under-replicated processor loop to sendThrottledReconstructionCommand so 1-1 replication continues when the cap is reached. Clear reconstruction counters on leader transition when pending ops are cleared. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I881fee0c3ea3c2e350b8fbb95321985569bef636
Skip reconstruction counter decrements when the command is no longer tracked after notifyStatusChanged clear, and floor decrements at zero. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I1530348aa2b09c232ad0410468d90cf098d8d0a3
…and tests. Validate reconstruction config bounds, remove a misleading processor test stub, and reuse ECUnderReplicationHandler.integers2ByteString in unit tests. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I55216c0c4c0af92e003486e04dd9bc0c42a8dc5c
…espace. Reserve the global reconstruction limit with a CAS before sending commands, release on send failure, and add a concurrent unit test. Clean up trailing whitespace in reconstruction limit tests. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: If8c0e4613209b8fb381271c7dab1733eac0e5bde
…ress review nits. Always reserve reconstruction slots via CAS even when the global limit is disabled so inflightReconstructionCount stays accurate for observability and runtime reconfiguration. Extend the default-disabled test, wrap long test lines, rename the concurrent-test command ID variable, and shut down the executor in a finally block. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I92d085a415582483d9baa3a8d42b1a956b42e485
…g ops. Register reconstructionCommandIdToPendingFragmentCount immediately after reserving a slot so opCompleted can always release it. Remove the map put from adjustPendingOpsAndMetrics and clear the entry on send failure. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I454d0a6d8510a200d93a26118cda8990416d149f
Return int via getOrDefault(cmdId, 0) and drop @VisibleForTesting from the package-private test accessor. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I679c3db47b8da51085b7f46e3cb5358ebf8c9864
|
Addressed @adoroszlai review from #10122:
The |
There was a problem hiding this comment.
Pull request overview
Adds SCM-side configuration knobs and an enforcement mechanism for a cluster-wide EC reconstruction concurrency cap in ReplicationManager, along with unit tests covering limit behavior, slot accounting, and failover/reset scenarios.
Changes:
- Add
ReplicationManagertracking for in-flight EC reconstruction commands (atomic counter + per-command pending fragment tracking) and enforcehdds.scm.replication.reconstruction.global.limitinsendThrottledReconstructionCommand(). - Introduce new SCM reconfigurable properties for EC decommission reconstruction switching (
*.enabled,*.load.factor) and validate their ranges. - Add tests to verify default-disabled behavior, limit enforcement/rejection, counter reset on leadership changes, and concurrent enforcement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java | Adds global EC reconstruction limit enforcement, in-flight tracking, new SCM configs, and validation/reset logic. |
| hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestReplicationManager.java | Adds unit tests covering the new global limit behavior, accounting, and concurrency/failover reset cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (ecDecommissionReconstructionLoadFactor < 0) { | ||
| throw new IllegalArgumentException( | ||
| "decommission.ec.reconstruction.load.factor is set to " | ||
| + ecDecommissionReconstructionLoadFactor + " and must be >= 0"); | ||
| } |
Note that per-volume thread pools (HDDS-15412) replace the outbound-counter and lookahead-dispatch approach; update Phase 1 PR link to apache#11054. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I5ce95991acd7c4d6854256fc9dd4365a91fd3143
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
Code review summary
Overall this is a solid foundation PR: atomic slot reservation, send-path enforcement, failover reset, and good unit coverage for success paths. A few issues are worth addressing before relying on the global cap in production decommission scenarios.
High — Global slots leak until event timeout on DN command failure
inflightReconstructionCount is decremented only in opCompleted() when a reconstruction ADD op completes or expires (ContainerReplicaPendingOps.removeExpiredEntries() → notifySubscribers(..., timedOut=true)).
DN failure reports do not clear pending ADD ops today (HDDS-15327). A failed reconstruction fragment therefore keeps its global slot until the default SCM event timeout (~12 minutes), even though the work is dead.
With reconstruction.global.limit > 0, a burst of failures during decommission can exhaust the cluster-wide cap for minutes — the same inflight-quota leak problem, now at cluster scope.
Suggestion: Either land HDDS-15327 first, or in this PR also decrement on failure/timeout paths that clear pending ops (or tie slot release to pending-op removal, not only successful opCompleted).
Medium — Empty missingContainerIndexes leaks a slot permanently
If fragmentCount == 0, a slot is reserved but no pending ADD ops are scheduled, so opCompleted() never runs and the slot is never released (failover clear excepted).
Production callers likely never send empty indexes, but sendThrottledReconstructionCommand() is public. A cheap guard would help:
if (fragmentCount == 0) {
releaseReconstructionSlot();
throw new IllegalArgumentException(...);
}Medium — Missing tests for timeout / failure slot release
Tests cover happy path, concurrent CAS, failover clear, and send failure before pending ops. Missing:
- Expired reconstruction ADD op →
opCompleted(..., timedOut=true)→ inflight count decrements - (Once HDDS-15327 lands) failure report → slot released promptly
These are the paths most likely to cause production stalls.
Low — Deferred metric conflates global vs per-DN overload
Both global-limit rejection and target-DN overload increment ec_reconstruction_cmds_deferred_total. Operators enabling the global cap won't be able to tell whether deferrals are from the cluster cap or datanode saturation. Consider a separate counter (HDDS-15075 territory).
Low — Runtime limit decrease can over-subscribe temporarily
If reconstruction.global.limit is lowered via reconfig while inflight > new limit, existing commands continue and new sends are blocked until natural completion. Reasonable behavior; worth a one-line note in the config description.
What looks good
- Default behavior unchanged when
global.limit=0 - Send-path enforcement keeps 1-1 replication running when cap is hit
- CAS loop in
tryReserveReconstructionSlot(); concurrent test validates cap=2 - Failover clears both map and counter when pending ops are wiped
finallyreleases slot + map entry if command never sentfragmentCountmatches pending ops scheduled inadjustPendingOpsAndMetrics()opCompletedignores unknown cmdIds after failover clear
Verdict: Approve with minor fixes recommended — guard fragmentCount == 0, and coordinate slot release on failure/timeout with HDDS-15327.
| + ") reached for container " + containerInfo.getContainerID()); | ||
| } | ||
| final long cmdId = command.getId(); | ||
| final int fragmentCount = command.getMissingContainerIndexes().size(); |
There was a problem hiding this 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.
| @@ -1059,6 +1142,22 @@ ReplicationQueue getQueue() { | |||
|
|
|||
| @Override | |||
| public void opCompleted(ContainerReplicaOp op, ContainerID containerID, boolean timedOut) { | |||
There was a problem hiding this 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.
| getAvailableDatanodesForReplication(targets); | ||
| if (targetWithCmds.isEmpty()) { | ||
| if (!tryReserveReconstructionSlot()) { | ||
| metrics.incrECReconstructionCmdsDeferredTotal(); |
There was a problem hiding this 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).
| defaultValue = "0", | ||
| reconfigurable = true, | ||
| tags = { SCM }, | ||
| description = "A cluster-wide limit to restrict the total number of " + |
There was a problem hiding this 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.
| } | ||
|
|
||
| @Test | ||
| public void testReconstructionGlobalLimitDisabledByDefault() |
There was a problem hiding this comment.
Medium — missing test coverage
Consider adding tests for:
- Expired reconstruction ADD op (
opCompleted(..., timedOut=true)) decrementsinflightReconstructionCount - (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.
There was a problem hiding this comment.
Thanks @jojochuang for the PR. Found two issues with AI review. PTAL
| 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(); | ||
| reconstructionCommandIdToPendingFragmentCount.put(cmdId, fragmentCount); | ||
| boolean sent = false; | ||
| try { | ||
| List<DatanodeDetails> targets = command.getTargetDatanodes(); | ||
| List<Pair<Integer, DatanodeDetails>> targetWithCmds = | ||
| getAvailableDatanodesForReplication(targets); | ||
| if (targetWithCmds.isEmpty()) { | ||
| metrics.incrECReconstructionCmdsDeferredTotal(); | ||
| throw new CommandTargetOverloadedException("No target with capacity " + | ||
| "available for reconstruction of " + containerInfo.getContainerID()); | ||
| } | ||
| DatanodeDetails target = selectAndOptionallyExcludeDatanode( | ||
| rmConf.getReconstructionCommandWeight(), targetWithCmds); | ||
| sendDatanodeCommand(command, containerInfo, target); | ||
| sent = true; | ||
| } finally { | ||
| if (!sent) { | ||
| reconstructionCommandIdToPendingFragmentCount.remove(cmdId); | ||
| releaseReconstructionSlot(); | ||
| } | ||
| } |
There was a problem hiding this comment.
P1 — failover can lose a reconstruction reservation
notifyStatusChanged() clears both the command map and the counter under serviceLock, but this method reserves/registers a command and then calls sendDatanodeCommand() without that lock. A sender can reserve/register, pause during the leadership transition, and resume after the new leader is ready. It then sends a command with the new term and schedules ADD ops after the map was cleared, so those pending fragments are never associated with a tracked command. The next command can therefore be admitted even though the old reconstruction is still active, exceeding the configured global cap. Please serialize the reset with reservation through the send path (or use an equivalent generation/recheck).
| 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(); | |
| reconstructionCommandIdToPendingFragmentCount.put(cmdId, fragmentCount); | |
| boolean sent = false; | |
| try { | |
| List<DatanodeDetails> targets = command.getTargetDatanodes(); | |
| List<Pair<Integer, DatanodeDetails>> targetWithCmds = | |
| getAvailableDatanodesForReplication(targets); | |
| if (targetWithCmds.isEmpty()) { | |
| metrics.incrECReconstructionCmdsDeferredTotal(); | |
| throw new CommandTargetOverloadedException("No target with capacity " + | |
| "available for reconstruction of " + containerInfo.getContainerID()); | |
| } | |
| DatanodeDetails target = selectAndOptionallyExcludeDatanode( | |
| rmConf.getReconstructionCommandWeight(), targetWithCmds); | |
| sendDatanodeCommand(command, containerInfo, target); | |
| sent = true; | |
| } finally { | |
| if (!sent) { | |
| reconstructionCommandIdToPendingFragmentCount.remove(cmdId); | |
| releaseReconstructionSlot(); | |
| } | |
| } | |
| serviceLock.lock(); | |
| try { | |
| if (!tryReserveReconstructionSlot()) { | |
| metrics.incrECReconstructionCmdsDeferredTotal(); | |
| throw new CommandTargetOverloadedException( | |
| "Global reconstruction limit (" + getReconstructionInFlightLimit() | |
| + ") reached for container " + containerInfo.getContainerID()); | |
| } | |
| final long cmdId = command.getId(); | |
| final int fragmentCount = command.getMissingContainerIndexes().size(); | |
| reconstructionCommandIdToPendingFragmentCount.put(cmdId, fragmentCount); | |
| boolean sent = false; | |
| try { | |
| List<DatanodeDetails> targets = command.getTargetDatanodes(); | |
| List<Pair<Integer, DatanodeDetails>> targetWithCmds = | |
| getAvailableDatanodesForReplication(targets); | |
| if (targetWithCmds.isEmpty()) { | |
| metrics.incrECReconstructionCmdsDeferredTotal(); | |
| throw new CommandTargetOverloadedException("No target with capacity " + | |
| "available for reconstruction of " + containerInfo.getContainerID()); | |
| } | |
| DatanodeDetails target = selectAndOptionallyExcludeDatanode( | |
| rmConf.getReconstructionCommandWeight(), targetWithCmds); | |
| sendDatanodeCommand(command, containerInfo, target); | |
| sent = true; | |
| } finally { | |
| if (!sent) { | |
| reconstructionCommandIdToPendingFragmentCount.remove(cmdId); | |
| releaseReconstructionSlot(); | |
| } | |
| } | |
| } finally { | |
| serviceLock.unlock(); | |
| } |
| if (ecDecommissionReconstructionLoadFactor < 0) { | ||
| throw new IllegalArgumentException( | ||
| "decommission.ec.reconstruction.load.factor is set to " | ||
| + ecDecommissionReconstructionLoadFactor + " and must be >= 0"); | ||
| } | ||
| if (ecDecommissionReconstructionLoadFactor > 1) { |
There was a problem hiding this comment.
P2 — reject non-finite load factors
Double.parseDouble() accepts NaN and infinities, and both range comparisons are false for NaN. The new decommission load factor can therefore be configured as NaN; the existing inflightReplicationLimitFactor has the same gap. Please add !Double.isFinite(...) to both validations.
| if (ecDecommissionReconstructionLoadFactor < 0) { | |
| throw new IllegalArgumentException( | |
| "decommission.ec.reconstruction.load.factor is set to " | |
| + ecDecommissionReconstructionLoadFactor + " and must be >= 0"); | |
| } | |
| if (ecDecommissionReconstructionLoadFactor > 1) { | |
| if (!Double.isFinite(ecDecommissionReconstructionLoadFactor) | |
| || ecDecommissionReconstructionLoadFactor < 0) { | |
| throw new IllegalArgumentException( | |
| "decommission.ec.reconstruction.load.factor is set to " | |
| + ecDecommissionReconstructionLoadFactor + " and must be >= 0"); | |
| } |
Apply the same finite-value guard to the existing inflightReplicationLimitFactor < 0 check above.
Summary
Reopens HDDS-15071 after rebase onto latest master (supersedes closed #10122).
Adds foundational SCM configuration and cluster-wide EC reconstruction throttling:
hdds.scm.replication.decommission.ec.reconstruction.enabled(default: false) — wired in HDDS-15072hdds.scm.replication.decommission.ec.reconstruction.load.factor(default: 0.9) — wired in HDDS-15072hdds.scm.replication.reconstruction.global.limit(default: 0 = disabled)This PR does not change EC decommission behavior by itself. It adds config, validation, inflight reconstruction tracking, and global throttling infrastructure. The dynamic replication→reconstruction switch is HDDS-15072.
Global limit enforcement:
sendThrottledReconstructionCommand()via atomic slot reservationDefault behavior is unchanged when
reconstruction.global.limit=0.Test plan
mvn -pl :hdds-server-scm -am test -Dtest=TestReplicationManager#testReconstruction* -DskipShade -DskipRecon -DskipDocsmvn -pl :hdds-server-scm -am test -Dtest=TestReplicationManager#testInflightReconstruction* -DskipShade -DskipRecon -DskipDocsHDDS-15072 depends on this PR.
Generated-by: Cursor (Auto)