smengcl commented on code in PR #10693:
URL: https://github.com/apache/ozone/pull/10693#discussion_r3642772503
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -719,6 +723,28 @@ private String reconfigReplicationStreamsLimit(String
value) {
return value;
}
+ private String reconfigPerVolumeStreamsLimit(String value) {
+ int newSize = Integer.parseInt(value);
+ Preconditions.checkArgument(newSize >= 1,
+ PER_VOLUME_STREAMS_LIMIT_KEY + " must be at least 1 but was %s",
+ value);
+ ReplicationConfig replicationConfig =
+ getDatanodeStateMachine().getSupervisor().getReplicationConfig();
+ if (!replicationConfig.isPerVolumeEnabled()) {
Review Comment:
Is the intent to allow this value to be staged while per-volume replication
is disabled? Returning the value makes reconfiguration report success and
updates the live `Configuration`, although no executor exists to resize. If
this is intentional, could the log or documentation make the
staged-until-restart behavior explicit? Otherwise, should the operation be
rejected?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -318,7 +320,9 @@ public String getNamespace() {
.register(OZONE_BLOCK_DELETING_SERVICE_TIMEOUT,
this::reconfigBlockDeletingServiceTimeout)
.register(REPLICATION_STREAMS_LIMIT_KEY,
- this::reconfigReplicationStreamsLimit);
+ this::reconfigReplicationStreamsLimit)
+ .register(PER_VOLUME_STREAMS_LIMIT_KEY,
Review Comment:
Registering `PER_VOLUME_STREAMS_LIMIT_KEY` changes the DataNode handler's
exact reconfigurable-property set, but
`TestDatanodeReconfiguration.reconfigurableProperties()` still omits this key.
Its equality assertion will fail when the integration test runs. Please update
the expected set and add runtime reconfiguration coverage for this property.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java:
##########
@@ -224,6 +230,29 @@ public static final class ReplicationConfig {
)
private double outOfServiceFactor = OUTOFSERVICE_FACTOR_DEFAULT;
+ @Config(key = PER_VOLUME_ENABLED_KEY,
+ type = ConfigType.BOOLEAN,
+ defaultValue = "false",
+ tags = {DATANODE},
+ description = "When true, push-based container replication uses a " +
+ "separate replication handler thread pool per data volume so " +
+ "that slow replication on one disk does not block replication " +
+ "on other disks. Pull replication and other replication tasks " +
+ "continue to use the global replication handler thread pool."
+ )
+ private boolean perVolumeEnabled = false;
+
+ @Config(key = PER_VOLUME_STREAMS_LIMIT_KEY,
+ type = ConfigType.INT,
+ defaultValue = "1",
+ reconfigurable = true,
+ tags = {DATANODE},
+ description = "The maximum number of concurrent push replication " +
Review Comment:
Once per-volume replication is enabled, normal push tasks no longer use the
global pool governed by `hdds.datanode.replication.streams.limit`; effective
push concurrency becomes approximately `data volume count ×
per.volume.streams.limit`, with out-of-service scaling applied separately. The
existing `streams.limit` description still calls it the maximum replication
concurrency per DataNode, which is no longer true in this mode. Could the
configuration description and decommission documentation explain this split and
the potential throughput change when enabling the feature?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationSupervisor.java:
##########
@@ -371,6 +501,20 @@ private void resize(HddsProtos.NodeOperationalState
nodeState) {
maxQueueSize = newMaxQueueSize;
executorThreadUpdater.accept(threadCount);
+
+ if (volumePools != null) {
Review Comment:
These per-volume executors add concurrent replication capacity, but
`ReplicationSupervisorMetrics.maxReplicationStreams` still calls
`getMaxReplicationStreams()`, which returns only the global executor size. Its
description claims to report the maximum number of concurrent replication
tasks. Could this metric aggregate the enabled pools, or could separate global
and per-volume capacity metrics be exposed?
--
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]
