sreejasahithi commented on code in PR #10489:
URL: https://github.com/apache/ozone/pull/10489#discussion_r3396234710
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerTask.java:
##########
@@ -679,6 +686,76 @@ public void testMoveSkippedWhenContainerStateChanged(State
invalidState)
assertEquals(initialSourceDelta,
diskBalancerService.getDeltaSizes().get(sourceVolume));
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testPendingDeletionDoesNotDropReplicasOnSameMillisecondKey(
Review Comment:
One optional improvement you could do: after delayed deletion, assert
sourceVolume.getCurrentUsage().getUsedSpace() returns to the value captured
before creating the containers.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerTask.java:
##########
@@ -679,6 +686,76 @@ public void testMoveSkippedWhenContainerStateChanged(State
invalidState)
assertEquals(initialSourceDelta,
diskBalancerService.getDeltaSizes().get(sourceVolume));
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testPendingDeletionDoesNotDropReplicasOnSameMillisecondKey(
+ ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ setLayoutAndSchemaForTest(versionInfo);
+
+ long delayMs = 2_000L;
+ diskBalancerService.setReplicaDeletionDelay(delayMs);
+
+ long id1 = CONTAINER_ID;
+ long id2 = CONTAINER_ID + 1;
+
+ Container c1 = createContainer(id1, sourceVolume, State.CLOSED);
+ Container c2 = createContainer(id2, sourceVolume, State.CLOSED);
+
+ File oldDir1 = new File(c1.getContainerData().getContainerPath());
+ File oldDir2 = new File(c2.getContainerData().getContainerPath());
+ assertTrue(oldDir1.exists());
+ assertTrue(oldDir2.exists());
+
+ // Reserve dest space like the choosing policy would.
+ destVolume.incCommittedBytes(c1.getContainerData().getBytesUsed());
+ destVolume.incCommittedBytes(c2.getContainerData().getBytesUsed());
+
+ // Schedule two moves (parallelThread default is 5 in config).
+ BackgroundTaskQueue queue = diskBalancerService.getTasks();
+ assertEquals(2, queue.size());
+ DiskBalancerService.DiskBalancerTask task1 =
+ (DiskBalancerService.DiskBalancerTask) queue.poll();
+ DiskBalancerService.DiskBalancerTask task2 =
+ (DiskBalancerService.DiskBalancerTask) queue.poll();
+ assertNotNull(task1);
+ assertNotNull(task2);
+
+ // Run both moves concurrently; fixed TestClock => same deadline key.
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+ try {
+ List<Future<BackgroundTaskResult>> futures =
pool.invokeAll(Arrays.asList(
+ task1::call,
+ task2::call));
+ for (Future<BackgroundTaskResult> future : futures) {
+ future.get(30, TimeUnit.SECONDS);
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+
+ assertEquals(2, diskBalancerService.getMetrics().getSuccessCount());
+
+ // With the bug: map size is 1; with fix: 2 queued replicas.
+ assertEquals(2, diskBalancerService.getPendingDeletionQueueSize());
Review Comment:
an optional improvement you could do:
```
assertEquals(1, diskBalancerService.getPendingDeletionDeadlineCount(),
"both moves should share one deadline key");
assertEquals(2, diskBalancerService.getPendingDeletionQueueSize(),
"both old replicas should be queued for deletion");
```
because size() on the map = number of deadline keys. With the bug you would
get 1 key and 1 queue element (one overwritten). With the fix you want 1 key
and 2 queue elements.
Here getPendingDeletionDeadlineCount can be a method added to
diskBalancerService similar to getPendingDeletionQueueSize
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##########
Review Comment:
stale comment
--
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]
