[rich7420]

What changes were proposed in this pull request?

Removing a RECOVERING container from ContainerSet can leave its recovery deadline in recoveringContainerMap until the background scrub removes it.

Clear the recovering entry in ContainerSet's shared removal method so deletion, failed-volume removal, and memory-only removal use the same cleanup. Clear the recovery entry before removing the container mapping, preserving the existing delete ordering. The close path keeps its existing cleanup.

Retain the force-delete regression test, verify recovery entries during memory-only removal and re-addition, and cover failed volumes with both CLOSED and RECOVERING containers.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16460

How was this patch tested?

• TestKeyValueHandler, TestContainerSet, TestVolumeSetDiskChecks, and TestContainerReader: 135 tests passed locally on JDK 21.
• checkstyle.sh: passed.
• With only the new tests applied to the previous PR head, the memory-only removal test failed for both container layouts and the RECOVERING volume-failure test failed because recovery entries remained.
• The original testDeleteRecoveringContainer regression also fails without the deletion cleanup.
• Fork CI passed.
• Updated-head 10×10 checks of TestContainerSet and TestVolumeSetDiskChecks passed.
• Previous-head TestKeyValueHandler 10×10 passed.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chihsuan]

Thanks for the patch! @rich7420 overall the fix looks good, but just curious, would it be worth moving the cleanup into ContainerSet's private removeContainer?

The volume failure path seems to leave the same stale entry:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java

Lines 454 to 466 in 3ccd783

	public void handleVolumeFailures(StateContext context) throws StorageContainerException {
	AtomicBoolean failedVolume = new AtomicBoolean(false);
	AtomicInteger containerCount = new AtomicInteger(0);
	for (Container<?> c : containerMap.values()) {
	ContainerData data = c.getContainerData();
	if (data.getVolume().isFailed()) {
	removeMissingContainer(data.getContainerID());
	LOG.debug("Removing Container {} as the Volume {} " +
	"has failed", data.getContainerID(), data.getVolume());
	failedVolume.set(true);
	containerCount.incrementAndGet();
	ContainerLogger.logLost(data, "Volume failure");
	}

[F64116045]

LGTM. Thanks for fixing this!

[sreejasahithi]

Thanks @rich7420 overall LGTM
just few non-blocking nit

[sreejasahithi]

nit: I think it would be better to update this javadoc aswell , as now it also calls removeRecoveringContainer

[sreejasahithi]

nit: you are already importing "org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State" , so we can drop this and use State.RECOVERING everywhere

[rich7420]

goood catch!

[rich7420]

The integration (container) failure is unrelated. TestBlockDeletingService#testBlockDeletion is flaky, tracked in HDDS-17257:

[ERROR]   TestBlockDeletingService.testBlockDeletion:628->lambda$testBlockDeletion$0:628->assertDeletionsInChecksumFile:1311 Failed to read container checksum tree file: /tmp/junit-11171629283323802431/hdds/eca15dd6-87ff-4680-9d35-46d58ca44679/current/containerDir1/117454920117519356/metadata/117454920117519356.tree

Merged master to re-run CI.