You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "chihsuan (via GitHub)" <gi...@apache.org> on 2026/10/23 09:17:39 UTC
[PR] HDDS-16455. Skip stale recovering scrub when container is no longer RECOVERING [ozone]
chihsuan opened a new pull request, #11713:
URL: https://github.com/apache/ozone/pull/11713
## What changes were proposed in this pull request?
The stale recovering scrub marks a container UNHEALTHY based only on its `recoveringContainerMap` entry. If the container was closed in the meantime, a valid reconstructed EC replica becomes UNHEALTHY and SCM has to reconstruct it again. This also covers HDDS-16456, where a write re-adds the entry right after the close.
This PR re-checks the state under the container write lock and only marks the container UNHEALTHY if it is still RECOVERING.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16455
## How was this patch tested?
https://github.com/chihsuan/ozone/actions/runs/39928857486
Added three tests to `TestStaleRecoveringContainerScrubbingService`:
- `testScrubSkippedWhenContainerClosedBeforeTaskRuns`: the container closes before the scrub task runs.
- `testScrubSkippedForStaleEntryOfClosedContainer`: a stale entry is left after the close.
- `testScrubDoesNotOverrideConcurrentClose`: a close runs while the scrub task is waiting on the lock.
Generated-by: Claude Code (Claude Opus 5.5)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16455. Skip stale recovering scrub when container is no longer RECOVERING [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16455. Skip stale recovering scrub when container is no longer RECOVERING [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #11713:
URL: https://github.com/apache/ozone/pull/11713#issuecomment-6384412445
Thanks @chihsuan for the patch, @F64116045, @smengcl for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16455. Skip stale recovering scrub when container is no longer RECOVERING [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #11713:
URL: https://github.com/apache/ozone/pull/11713
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16455. Skip stale recovering scrub when container is no longer RECOVERING [ozone]
Posted by "F64116045 (via GitHub)" <gi...@apache.org>.
F64116045 commented on code in PR #11713:
URL: https://github.com/apache/ozone/pull/11713#discussion_r4366108063
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/StaleRecoveringContainerScrubbingService.java:
##########
Review Comment:
nit: Here still says this service deletes stale recovering containers, which is outdated.
Wasn't introduced by this PR, so I think it's also fine to leave it for a follow-up.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org