Copilot commented on code in PR #11713:
URL: https://github.com/apache/ozone/pull/11713#discussion_r4358128840
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestStaleRecoveringContainerScrubbingService.java:
##########
@@ -237,4 +241,98 @@ public void
testScrubSkippedWhenDeadlineExtendedBeforeTaskRuns(
assertEquals(RECOVERING,
containerSet.getContainer(containerId).getContainerState());
assertTrue(containerSet.getRecoveringContainerMap().containsKey(containerId));
}
+
+ @ContainerTestVersionInfo.ContainerTest
+ public void testScrubSkippedWhenContainerClosedBeforeTaskRuns(
+ ContainerTestVersionInfo versionInfo) throws Exception {
+ initVersionInfo(versionInfo);
+ ContainerSet containerSet = newContainerSet(1000, testClock);
+ StaleRecoveringContainerScrubbingService srcss =
+ new StaleRecoveringContainerScrubbingService(
+ 50, TimeUnit.MILLISECONDS, 10,
+ Duration.ofSeconds(300).toMillis(),
+ containerSet);
+ List<Long> ids = createTestContainers(containerSet, 1, RECOVERING);
+ long containerId = ids.get(0);
+ Container<?> container = containerSet.getContainer(containerId);
+ testClock.fastForward(1000L);
+ BackgroundTaskQueue tasks = srcss.getTasks();
+ assertFalse(tasks.isEmpty());
+
+ container.markContainerForClose();
+ container.close();
+ while (!tasks.isEmpty()) {
+ BackgroundTask task = tasks.poll();
+ task.call();
+ }
+ assertEquals(CLOSED, container.getContainerState());
+ }
+
+ @ContainerTestVersionInfo.ContainerTest
+ public void testScrubSkippedForStaleEntryOfClosedContainer(
+ ContainerTestVersionInfo versionInfo) throws Exception {
+ initVersionInfo(versionInfo);
+ ContainerSet containerSet = newContainerSet(1000, testClock);
+ StaleRecoveringContainerScrubbingService srcss =
+ new StaleRecoveringContainerScrubbingService(
+ 50, TimeUnit.MILLISECONDS, 10,
+ Duration.ofSeconds(300).toMillis(),
+ containerSet);
+ List<Long> ids = createTestContainers(containerSet, 1, RECOVERING);
+ long containerId = ids.get(0);
+ Container<?> container = containerSet.getContainer(containerId);
+ containerSet.removeRecoveringContainer(containerId);
+ container.markContainerForClose();
+ container.close();
+ // A write that checked RECOVERING before the close re-inserts the entry
afterwards.
+ containerSet.updateRecoveringContainerTimeout(containerId);
+ assertEquals(CLOSED, container.getContainerState());
+
assertTrue(containerSet.getRecoveringContainerMap().containsKey(containerId));
+
+ testClock.fastForward(1000L);
+ srcss.runPeriodicalTaskNow();
+ assertEquals(CLOSED, container.getContainerState());
+
assertFalse(containerSet.getRecoveringContainerMap().containsKey(containerId));
+ }
+
+ @ContainerTestVersionInfo.ContainerTest
+ public void testScrubDoesNotOverrideConcurrentClose(
+ ContainerTestVersionInfo versionInfo) throws Exception {
+ initVersionInfo(versionInfo);
+ ContainerSet containerSet = newContainerSet(1000, testClock);
+ StaleRecoveringContainerScrubbingService srcss =
+ new StaleRecoveringContainerScrubbingService(
+ 50, TimeUnit.MILLISECONDS, 10,
+ Duration.ofSeconds(300).toMillis(),
+ containerSet);
+ List<Long> ids = createTestContainers(containerSet, 1, RECOVERING);
+ long containerId = ids.get(0);
+ Container<?> container = containerSet.getContainer(containerId);
+ testClock.fastForward(1000L);
+ BackgroundTaskQueue tasks = srcss.getTasks();
+ assertFalse(tasks.isEmpty());
+ BackgroundTask task = tasks.poll();
+
+ // Hold the container lock like a close in progress, so the scrub task
blocks on it.
+ AtomicReference<Exception> error = new AtomicReference<>();
+ Thread scrubber = new Thread(() -> {
+ try {
+ task.call();
+ } catch (Exception e) {
+ error.set(e);
+ }
+ });
+ container.writeLock();
+ try {
+ scrubber.start();
+ GenericTestUtils.waitFor(() -> scrubber.getState() ==
Thread.State.WAITING, 10, 10000);
Review Comment:
`Thread.getState() == WAITING` is a brittle synchronization mechanism and
can be JVM/implementation dependent (the thread may appear as `BLOCKED` /
`TIMED_WAITING` while contending on the lock). To make this test deterministic,
prefer explicit coordination (eg, a `CountDownLatch`/barrier signaled from
within the task right before attempting to acquire the container lock), or at
least accept multiple states that indicate lock contention.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/StaleRecoveringContainerScrubbingService.java:
##########
@@ -89,7 +91,18 @@ public BackgroundTaskResult call() throws Exception {
}
Container con = containerSet.getContainer(containerID);
if (null != con) {
- con.markContainerUnhealthy();
+ con.writeLock();
+ try {
+ // The container may have left RECOVERING after this task was
scheduled.
+ if (con.getContainerState() != RECOVERING) {
+ LOG.debug("Skipping stale recovering scrub for container {} -
state is {}",
+ containerID, con.getContainerState());
Review Comment:
`con.getContainerState()` is called twice inside the lock. Capturing it once
in a local variable (eg, `state`) improves readability and guarantees the
logged value matches the checked value.
--
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]
