smengcl commented on code in PR #10693:
URL: https://github.com/apache/ozone/pull/10693#discussion_r3671603130
##########
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 this key still breaks
`TestDatanodeReconfiguration.reconfigurableProperties()`, whose exact expected
set omits `PER_VOLUME_STREAMS_LIMIT_KEY`. This can be reproduced on the current
head: 6 tests run and this test fails because the actual set contains
`hdds.datanode.replication.per.volume.streams.limit`. Please add the key to the
expected set.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java:
##########
@@ -183,16 +183,27 @@ public static final class ReplicationConfig {
static final String REPLICATION_OUTOFSERVICE_FACTOR_KEY =
PREFIX + "." + OUTOFSERVICE_FACTOR_KEY;
+ public static final String PER_VOLUME_ENABLED_KEY =
+ PREFIX + ".per.volume.enabled";
+ public static final String PER_VOLUME_STREAMS_LIMIT_KEY =
+ PREFIX + ".per.volume.streams.limit";
+ public static final int PER_VOLUME_STREAMS_LIMIT_DEFAULT = 2;
+
/**
- * The maximum number of replication commands a single datanode can execute
- * simultaneously.
+ * Maximum concurrent tasks on the global replication handler thread pool.
*/
@Config(key = "hdds.datanode.replication.streams.limit",
type = ConfigType.INT,
defaultValue = "10",
tags = {DATANODE},
- description = "The maximum number of replication commands a single " +
- "datanode can execute simultaneously"
+ description = "Maximum concurrent replication tasks on the global "
Review Comment:
This key still sizes two executors: the target-side `ReplicationServer` gRPC
executor and the `ReplicationSupervisor` global task executor; runtime
reconfiguration also resizes both. The per-volume key only changes source-side
push scheduling, so inbound push concurrency remains capped by `streams.limit`.
Could the description and documentation retain both roles so operators know
which limit to tune?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestReplicationSupervisor.java:
##########
@@ -1052,4 +1070,449 @@ private void scheduleTasks(
rs.addTask(new ReplicationTask(toTarget(i, target), noopReplicator));
}
}
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumeDisabledUsesGlobalPool(ContainerLayoutVersion layout) {
+ this.layoutVersion = layout;
+ ReplicationServer.ReplicationConfig repConf =
+ new ReplicationServer.ReplicationConfig();
+ repConf.setPerVolumeEnabled(false);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .executor(newDirectExecutorService())
+ .clock(clock)
+ .build();
+
+ try {
+ assertNull(supervisor.getVolumeReplicationThreadPools());
+ replicatorRef.set(doneReplicator);
+ supervisor.addTask(createTask(1L));
+ assertEquals(1, supervisor.getReplicationSuccessCount());
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumeInitLogging(ContainerLayoutVersion layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 1);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ LogCapturer supervisorLogs =
+ LogCapturer.captureLogs(ReplicationSupervisor.class);
+ LogCapturer poolLogs =
+ LogCapturer.captureLogs(VolumeReplicationThreadPools.class);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .executor(newDirectExecutorService())
+ .clock(clock)
+ .build();
+
+ try {
+ assertNotNull(supervisor.getVolumeReplicationThreadPools());
+ assertThat(supervisorLogs.getOutput())
+ .contains("Per-volume container replication thread pools enabled");
+ assertThat(poolLogs.getOutput())
+ .contains("Initialized 2 per-volume replication thread pools");
+ for (StorageVolume volume : volumeSet.getVolumesList()) {
+ assertThat(poolLogs.getOutput())
+ .contains(volume.getStorageDir().getPath());
+ }
+ } finally {
+ supervisorLogs.stopCapturing();
+ poolLogs.stopCapturing();
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumePoolSizeRespected(ContainerLayoutVersion layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 3);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .executor(newDirectExecutorService())
+ .clock(clock)
+ .build();
+
+ try {
+ VolumeReplicationThreadPools pools =
+ supervisor.getVolumeReplicationThreadPools();
+ assertNotNull(pools);
+ for (StorageVolume volume : volumeSet.getVolumesList()) {
+ assertEquals(3, pools.getPoolSize(volume.getStorageDir().getPath()));
+ }
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumePoolResize(ContainerLayoutVersion layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 1);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .executor(newDirectExecutorService())
+ .clock(clock)
+ .build();
+
+ try {
+ supervisor.setPerVolumePoolSize(3);
+ VolumeReplicationThreadPools pools =
+ supervisor.getVolumeReplicationThreadPools();
+ for (StorageVolume volume : volumeSet.getVolumesList()) {
+ assertEquals(3, pools.getPoolSize(volume.getStorageDir().getPath()));
+ }
+ assertEquals(3, repConf.getPerVolumeStreamsLimit());
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumePoolResizeOnNodeStateChange(ContainerLayoutVersion
layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 2);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .clock(clock)
+ .build();
+
+ try {
+ datanode.setPersistedOpState(IN_SERVICE);
+ supervisor.nodeStateUpdated(
+ HddsProtos.NodeOperationalState.DECOMMISSIONING);
+ VolumeReplicationThreadPools pools =
+ supervisor.getVolumeReplicationThreadPools();
+ int expected = repConf.scaleOutOfServiceLimit(2);
+ for (StorageVolume volume : volumeSet.getVolumesList()) {
+ assertEquals(expected,
+ pools.getPoolSize(volume.getStorageDir().getPath()));
+ }
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumePoolResizeDuringDecommission(ContainerLayoutVersion
layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 2);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .clock(clock)
+ .build();
+
+ try {
+ datanode.setPersistedOpState(IN_SERVICE);
+ supervisor.nodeStateUpdated(DECOMMISSIONING);
+ supervisor.setPerVolumePoolSize(2);
+ VolumeReplicationThreadPools pools =
+ supervisor.getVolumeReplicationThreadPools();
+ int expected = repConf.scaleOutOfServiceLimit(2);
+ for (StorageVolume volume : volumeSet.getVolumesList()) {
+ assertEquals(expected,
+ pools.getPoolSize(volume.getStorageDir().getPath()));
+ }
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void nonPushReplicationUsesGlobalPoolWhenPerVolumeEnabled(
+ ContainerLayoutVersion layout, @TempDir File perVolumeTempDir) throws
Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 1);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+ AtomicInteger globalExecutions = new AtomicInteger();
+
+ ExecutorService trackingGlobal = new AbstractExecutorService() {
+ @Override
+ public void shutdown() {
+ }
+
+ @Override
+ public List<Runnable> shutdownNow() {
+ return emptyList();
+ }
+
+ @Override
+ public boolean isShutdown() {
+ return false;
+ }
+
+ @Override
+ public boolean isTerminated() {
+ return false;
+ }
+
+ @Override
+ public boolean awaitTermination(long timeout, TimeUnit unit) {
+ return true;
+ }
+
+ @Override
+ public void execute(Runnable command) {
+ globalExecutions.incrementAndGet();
+ command.run();
+ }
+ };
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .executor(trackingGlobal)
+ .clock(clock)
+ .build();
+
+ try {
+ supervisor.addTask(createReconciliationTask(1L));
+ assertEquals(1, globalExecutions.get());
+ assertEquals(1, supervisor.getReplicationSuccessCount());
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void perVolumePushIsolation(ContainerLayoutVersion layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 1);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ HddsVolume vol1 = (HddsVolume) volumeSet.getVolumesList().get(0);
+ HddsVolume vol2 = (HddsVolume) volumeSet.getVolumesList().get(1);
+
+ addContainerOnVolume(1L, vol1, conf);
+ addContainerOnVolume(2L, vol2, conf);
+
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ CountDownLatch vol1Started = new CountDownLatch(1);
+ CountDownLatch vol1Release = new CountDownLatch(1);
+ ContainerReplicator volumeAwareReplicator = task -> {
+ Container<?> container = set.getContainer(task.getContainerId());
+ HddsVolume volume = container.getContainerData().getVolume();
+ if (volume == vol1) {
+ vol1Started.countDown();
+ try {
+ assertTrue(vol1Release.await(10, TimeUnit.SECONDS));
+ } catch (InterruptedException ie) {
+ Thread.currentThread().interrupt();
+ throw new AssertionError(ie);
+ }
+ }
+ task.setStatus(DONE);
+ };
+ replicatorRef.set(volumeAwareReplicator);
+
+ ReplicationSupervisor supervisor = ReplicationSupervisor.newBuilder()
+ .stateContext(context)
+ .replicationConfig(repConf)
+ .containerSet(set)
+ .volumeSet(volumeSet)
+ .clock(clock)
+ .build();
+
+ try {
+ supervisor.addTask(createPushTask(1L));
+ assertTrue(vol1Started.await(10, TimeUnit.SECONDS));
+
+ supervisor.addTask(createPushTask(2L));
+ GenericTestUtils.waitFor((BooleanSupplier) () ->
+ supervisor.getReplicationSuccessCount() >= 1, 100, 10000);
+
+ assertEquals(1, supervisor.getReplicationSuccessCount());
+ vol1Release.countDown();
+ GenericTestUtils.waitFor((BooleanSupplier) () ->
+ supervisor.getReplicationSuccessCount() == 2, 100, 10000);
+ } finally {
+ supervisor.stop();
+ }
+ }
+
+ @ContainerLayoutTestInfo.ContainerTest
+ public void volumeFailureCleansUpQueuedTasks(ContainerLayoutVersion layout,
+ @TempDir File perVolumeTempDir) throws Exception {
+ this.layoutVersion = layout;
+ OzoneConfiguration conf = perVolumeConf(perVolumeTempDir, 1);
+ MutableVolumeSet volumeSet = newVolumeSet(conf);
+ HddsVolume vol1 = (HddsVolume) volumeSet.getVolumesList().get(0);
+ addContainerOnVolume(1L, vol1, conf);
+ addContainerOnVolume(2L, vol1, conf);
+
+ ReplicationServer.ReplicationConfig repConf =
+ conf.getObject(ReplicationServer.ReplicationConfig.class);
+
+ CountDownLatch task1Started = new CountDownLatch(1);
+ CountDownLatch task1Block = new CountDownLatch(1);
+ replicatorRef.set(task -> {
+ if (task.getContainerId() == 1L) {
+ task1Started.countDown();
+ try {
+ assertTrue(task1Block.await(30, TimeUnit.SECONDS));
Review Comment:
`shutdownFailedVolumePools()` calls `shutdownNow()`, so interrupting this
blocking task is expected. Re-throwing `AssertionError` happens on the executor
thread and is not observed by JUnit—the targeted test still reports success.
Could this either treat interruption as the expected outcome or propagate the
worker result back to the test thread for assertion?
--
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]
