HDDS-13034. Refactor Directory Deleting Service to use ReclaimableDirectoryFilter & ReclaimableKeyFilter

[jojochuang]

What changes were proposed in this pull request?

With implementation of ReclaimableKeyFilter & ReclaimableDirectoryFilter both snapshot directory deep cleaning and AOS DirectoryDeletingService can be unified which will simplify the entire implementation rather than having multiple implementations for DirectoryDeletingService.
Algorithm in play:

1. The directory deleting service would be iterate through all the entries in the deletedDirectoryTable for a store(AOS and snapshot). One thread processing deleted directory entry for a particular store.

2. This thread would further initialize a DeletedDirectorySupplier which is an implementation for thread safe table iterator(We can eventually use a TableSpliterator after HDDS-12742. Make RDBStoreAbstractIterator Return Reference-Counted KeyValues apache/ozone#8203 gets merged).

3. Each thread here would be iterating through the deletedDirectoryTable independently and would submit purgeDirectoriesRequests where the logic for directoryPurge is as follows:
   - Check if the deletedDirectory object from deletedDirectoryTable can be reclaimed as in check if the reference for directory object exists in the previous directory.
   - If the directory exists in the previous snapshot then we need not expand the sub files inside the files[Number of files would be a lot generally (number of files >>> number of directories)] and we would just expand all the subdirectories under the directory(this would help in the recursively iterating on the sub files in the next iteration instead of traversing again from root all over). Now we cannot purge the directories as a reference exists we would just move all sub directories present in the AOS directory table and files which can be reclaimed i.e. files which are not present in the previous snapshot's fileTable by iterating through the sub file list based on deleted directory prefix in the AOS file table and move it to the deleted space of the store[Check HDDS-11244. OmPurgeDirectoriesRequest should clean up File and Directory tables of AOS for deleted snapshot directories apache/ozone#8509].
   - If the directory doesn't exist in the previous snapshot we have to blindly remove all entries[sub files & sub directories] inside this directory irrespective whether it can be reclaimed or not and purge the directory if all the entries are going to be moved from AOS to the specific store's deleted space of the store.

4. Now once all threads have processed if none of the threads have been able to move any entry from the key space to deleted space(All keys already have been reclaimed in the previous run) we can go ahead update the exclusive size of the previous snapshot and the deep clean flag of the snapshot.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing unit tests and additional assertions added on existing tests.

[gemini-code-assist]

Hello @jojochuang, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! gemini-code-assist here, providing a summary of this pull request.

The primary goal of this PR, as stated in the title and description, is to refactor the Directory Deleting Service (DDS) to unify the deep cleaning process for both snapshots and the Active Object Store (AOS). This unification is achieved by leveraging the ReclaimableDirectoryFilter and ReclaimableKeyFilter classes. The PR outlines an algorithm where the DDS iterates through deleted directory entries, initializes a thread-safe iterator, and submits purge requests. The logic for purging directories and files depends on whether they are still referenced by a previous snapshot. If a directory is referenced, only its subdirectories are expanded for future iterations, and reclaimable sub-files are moved to deleted space. If not referenced, all contents are removed. The process also includes updating snapshot exclusive size and deep clean flags once processing is complete for a store (AOS or snapshot).

Highlights

• Directory Deleting Service Refactor: The Directory Deleting Service has been significantly refactored to use ReclaimableDirectoryFilter and ReclaimableKeyFilter. This allows the same service to handle deep cleaning for both the Active Object Store and individual snapshots, unifying the logic and simplifying the implementation.
• Snapshot Deep Cleaning Unification: The logic for deep cleaning directories within snapshots is now integrated into the main Directory Deleting Service, replacing the separate SnapshotDirectoryCleaningService. This streamlines the background deletion processes.
• Deprecated Configurations: Configuration keys specifically for the old SnapshotDirectoryCleaningService (ozone.snapshot.directory.service.interval and ozone.snapshot.directory.service.timeout) have been marked as deprecated in OMConfigKeys.java and ozone-default.xml.
• Container Balancer Validation: A new validation has been added to the Container Balancer configuration to ensure that the difference between hdds.container.balancer.move.timeout, hdds.container.balancer.move.replication.timeout, and hdds.scm.replication.event.timeout.datanode.offset is at least 9 minutes.
• Debug Tool Enhancements: The ozone debug replicas verify command now includes an option (--container-state) to check the state of containers and their replicas, identifying those in unhealthy or deleted states. A cache size option (--container-cache-size) was also added for this check.
• Freon Bucket Layout Option: The Freon RandomKeyGenerator tool now supports specifying the bucket layout (--bucket-layout) when creating buckets, allowing for testing with different bucket types like FILE_SYSTEM_OPTIMIZED.

Changelog

Click here to see the changelog

• hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
  • Updated the default value for fs.trash.classname from org.apache.hadoop.ozone.om.TrashPolicyOzone to org.apache.hadoop.fs.ozone.OzoneTrashPolicy (line 624).
• hadoop-hdds/common/src/main/resources/ozone-default.xml
  • Updated the default value for fs.trash.classname property to org.apache.hadoop.fs.ozone.OzoneTrashPolicy (line 2792).
  • Added DEPRECATED tag to the ozone.snapshot.directory.service.timeout property description (line 3789).
  • Added DEPRECATED tag and prefix to the description for ozone.snapshot.directory.service.interval property (lines 3798-3800).
• hadoop-hdds/docs/content/interface/Ofs.md
  • Updated the fs.trash.classname value in the documentation example to org.apache.hadoop.fs.ozone.OzoneTrashPolicy (line 240).
• hadoop-hdds/docs/content/interface/Ofs.zh.md
  • Updated the fs.trash.classname value in the Chinese documentation example to org.apache.hadoop.fs.ozone.OzoneTrashPolicy (line 223).
• hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
  • Added imports for Duration and TimeUnit (lines 22, 24).
  • Added a new validation check in validateConfiguration to ensure sufficient time difference between balancer timeouts (lines 465-480).
• hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java
  • Added a new test case to verify the validation of balancer timeout configurations, specifically the 9-minute minimum difference (lines 210-226).
• hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
  • Marked OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL and OZONE_SNAPSHOT_DIRECTORY_SERVICE_TIMEOUT constants as @Deprecated (lines 390, 393, 396, 399).
• hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug-tests.robot
  • Removed unused variable ${OM_SERVICE_ID} (line 30).
  • Added --container-state and --block-existence options to the ozone debug replicas verify command in the test case (line 40).
• hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/RandomKeyGenerator.java
  • Added imports for BucketArgs and BucketLayout (lines 63, 71).
  • Added a new command-line option --bucket-layout to specify the bucket layout (lines 196-200).
  • Added a @VisibleForTesting method getBucketMapSize to return the size of the buckets map (lines 657-665).
  • Modified createBucket method to use BucketArgs if --bucket-layout is specified (lines 778-785).
  • Added @VisibleForTesting annotation to the getBucket method (line 908).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java
  • Removed import for TrashPolicyOzone (line 114).
  • Updated assertion in testTrash to check for OzoneTrashPolicy.class instead of TrashPolicyOzone.class (line 1687).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java
  • Removed import for TrashPolicyOzone (line 120).
  • Updated assertion in testTrash to check for OzoneTrashPolicy.class instead of TrashPolicyOzone.class (line 1871).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestContainerBalancerOperations.java
  • Adjusted the default value for moveReplicationTimeout in test cases (lines 85, 150).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestDirectoryDeletingServiceWithFSO.java
  • Added imports for ReclaimableDirFilter and ReclaimableKeyFilter (lines 82, 83).
  • Replaced assertThat(elapsedRunCount).isGreaterThanOrEqualTo(7) with GenericTestUtils.waitFor for more robust waiting in testDeleteWithLargeSubPathsThanBatchSize (line 282).
  • Updated the constructor call for DirectoryDeletingService in testAOSKeyDeletingWithSnapshotCreateParallelExecution to include the deepCleanSnapshots parameter (line 595).
  • Updated the mocked optimizeDirDeletesAndSubmitRequest method call to include the new filter parameters (lines 629-630).
  • Changed the mocked method call from getSnapshot to getActiveSnapshot in testAOSKeyDeletingWithSnapshotCreateParallelExecution (line 654).
  • Removed a blank line (line 732).
  • Corrected assertions in testDirDeletedTableCleanUpForSnapshot for dirTable count (from 4 to 0) and deletedDirTable count (from 1 to 5) after directory deletion (lines 757, 765).
  • Corrected assertion for getMovedDirsCount in testDirDeletedTableCleanUpForSnapshot (from 0 to 4) (line 773).
  • Updated the constructor call for DirectoryDeletingService in getMockedDirectoryDeletingService to include the deepCleanSnapshots parameter (line 530).
  • Added mocking for KeyManager and its getDeletedDirEntries method in getMockedDirectoryDeletingService (lines 525-526, 539).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSnapshotDeletingServiceIntegrationTest.java
  • Added import for IOException (line 27).
  • Adjusted the timeout for OZONE_SNAPSHOT_DELETING_SERVICE_TIMEOUT from 10000ms to 10ms and removed the setting for OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL (lines 130-131).
  • Added assertions for initial table counts and suspended KDS/DDS before creating keys (lines 259-264).
  • Added resume calls for KDS/DDS and waits for deep cleaning flags to be set after creating snapshots (lines 388-401, 444-457).
  • Added assertions for table counts after deep cleaning and before deleting snap2 (lines 408-411).
  • Added resume calls for KDS/DDS after deleting snap3 and updated assertions for deleted/renamed table counts (lines 472-478).
  • Added mocking for KeyManager and its getDeletedDirEntries method in getMockedDirectoryDeletingService (lines 525-526, 539).
  • Updated the constructor call for DirectoryDeletingService in getMockedDirectoryDeletingService to include the deepCleanSnapshots parameter (line 530).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDirectoryCleaningService.java
  • Added import for IOException (line 27).
  • Added imports for OmMetadataManagerImpl and SnapshotChainManager (lines 48, 49).
  • Replaced import for SnapshotDirectoryCleaningService with DirectoryDeletingService (line 55).
  • Changed the configuration key used for the interval from OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL to OZONE_DIR_DELETING_SERVICE_INTERVAL (line 82).
  • Replaced SnapshotDirectoryCleaningService with DirectoryDeletingService and added SnapshotChainManager in the test setup (lines 146-149).
  • Replaced references to snapshotDirectoryCleaningService with directoryDeletingService and adjusted the wait condition for run count (lines 228-230).
  • Added a wait condition within the snapshot iteration to ensure the next snapshot in the chain is deep cleaned before checking exclusive size (lines 247-255).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugShell.java
  • Added --container-state option to the ozone debug replicas verify command in the test case (line 106).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
  • Added import for OzoneTrashPolicy (line 72).
  • Removed import for TrashPolicyOzone (line 95).
  • Updated comments and code to refer to OzoneTrashPolicy instead of TrashPolicyOzone (lines 994, 998, 1004, 1189).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManager.java
  • Removed import for SnapshotDirectoryCleaningService (line 44).
  • Added a default method getDeletedDirEntries() that calls the overloaded version with nulls (lines 276-280).
  • Modified the signature of getPendingDeletionSubDirs to accept a filter parameter (lines 309-310).
  • Modified the signature of getPendingDeletionSubFiles to accept a filter parameter (lines 321-322).
  • Removed the getSnapshotDirectoryService method (lines 355-360).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
  • Removed imports for deprecated snapshot directory service config keys (lines 61-64).
  • Added import for Table.KeyValue (line 130).
  • Added import for WithParentObjectId (line 160).
  • Removed import for SnapshotDirectoryCleaningService (line 169).
  • Removed the snapshotDirectoryCleaningService field (line 215).
  • Updated the constructor call for DirectoryDeletingService to include the isSnapshotDeepCleaningEnabled parameter (line 289).
  • Removed the logic for starting the snapshotDirectoryCleaningService (lines 351-365).
  • Removed the logic for shutting down the snapshotDirectoryCleaningService (lines 444-447).
  • Updated the signature of getPendingDeletionKeys to use KeyValue instead of Table.KeyValue (lines 713, 721).
  • Updated the type of the iterator in getPendingDeletionKeys to use KeyValue (line 727).
  • Updated the type of the kv variable in getPendingDeletionKeys to use KeyValue (line 739).
  • Updated the signature of getTableEntries to use KeyValue instead of Table.KeyValue (lines 774, 777, 779).
  • Updated the type of the kv variable in getTableEntries to use KeyValue (line 790).
  • Updated the signature of getRenamesKeyEntries to use KeyValue instead of Table.KeyValue (lines 812, 814).
  • Updated the type of the iterator in getRenamesKeyEntries to use KeyValue (line 816).
  • Updated the signature of getDeletedKeyEntries to use KeyValue instead of Table.KeyValue (lines 861, 863).
  • Updated the type of the iterator in getDeletedKeyEntries to use KeyValue (line 866).
  • Removed the getSnapshotDirectoryService method (lines 956-959).
  • Updated the type of the iterator in createFakeDirIfShould to use KeyValue (line 1511).
  • Updated the type of the keyValue variable in createFakeDirIfShould to use KeyValue (line 1514).
  • Updated the type of the iterator in listStatus to use KeyValue (line 1825).
  • Updated the signature of getIteratorForKeyInTableCache to use KeyValue instead of Table.KeyValue (lines 1882, 1887).
  • Updated the signature of findKeyInDbWithIterator to use KeyValue instead of Table.KeyValue (lines 1905, 1910).
  • Updated the signature of getDeletedDirEntries to use KeyValue instead of Table.KeyValue (line 2161).
  • Refactored getPendingDeletionSubDirs and getPendingDeletionSubFiles to use a new private helper method gatherSubPathsWithIterator (lines 2168-2229).
  • Added the private helper method gatherSubPathsWithIterator to handle iterating and filtering sub-paths (lines 2177-2213).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
  • Included om.getKeyManager().getDirDeletingService() and removed om.getKeyManager().getSnapshotDirectoryService() from the list of services to lock during checkpointing (lines 707, 709).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
  • Changed the class visibility from public to package-private (class TrashPolicyOzone) (line 51).
  • Removed the default constructor TrashPolicyOzone() (lines 65-66).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java
  • Added static helper methods getKeyInfoWithFullPath to create OmKeyInfo with full path from OmDirectoryInfo or existing OmKeyInfo (lines 723-737).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java
  • Added imports for Optional and StringUtils (lines 31, 37).
  • Added imports for OMResponse and CheckedFunction (lines 63, 70).
  • Removed InterruptedException from the throws clause of processKeyDeletes (line 110).
  • Removed InterruptedException from the throws clause of submitPurgeKeysRequest (line 150).
  • Updated the throws clause of submitRequest to include InterruptedException (line 245).
  • Changed the return type of submitPurgePaths from void to OMResponse and added bootstrap lock acquisition (lines 249, 274-279).
  • Modified the signature of prepareDeleteDirRequest to accept a purgeDir boolean, a reclaimableFileFilter, and return Optional<PurgePathRequest> (lines 312-317).
  • Updated the call to keyManager.getPendingDeletionSubDirs in prepareDeleteDirRequest to pass a filter that always returns true (line 331).
  • Updated the call to keyManager.getPendingDeletionSubFiles in prepareDeleteDirRequest to pass a filter based on purgeDir or the reclaimableFileFilter (line 349).
  • Modified the logic in prepareDeleteDirRequest to return Optional.empty() if no sub-paths are found and the directory is not purged (lines 362-364).
  • Updated the signature of optimizeDirDeletesAndSubmitRequest to accept reclaimableDirChecker and reclaimableFileChecker filters (lines 376-377).
  • Modified the loop in optimizeDirDeletesAndSubmitRequest to use the new prepareDeleteDirRequest signature and handle the Optional return value (lines 388-405).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java
  • Added imports for various utility and concurrency classes (Maps, Closeable, Collection, Iterator, Map, CompletableFuture, ExecutionException, ExecutorService, LinkedBlockingDeque, ThreadPoolExecutor, Stream, StringUtils) (lines 21-40).
  • Added imports for KeyManager, SnapshotChainManager, IOzoneManagerLock, ReclaimableDirFilter, ReclaimableKeyFilter, OzoneManagerProtocolProtos (lines 51, 57, 60, 62, 63, 64).
  • Removed dirDeletingCorePoolSize, deletedDirSupplier, and taskCount fields (lines 79, 84, 86).
  • Added snapshotChainManager, deepCleanSnapshots, deletionThreadPool, and numberOfParallelThreadsPerStore fields (lines 98-101).
  • Updated the constructor to accept deepCleanSnapshots and initialize the thread pool (lines 105-114).
  • Removed the getTaskCount method (lines 118-120).
  • Modified getTasks to create one task for AOS (snapshotId=null) and one task for each snapshot if deep cleaning is enabled (lines 158-171).
  • Removed the override of the shutdown method (lines 177-179).
  • Refactored DeletedDirSupplier to be a static inner class implementing Closeable and taking the iterator in its constructor (lines 181-199).
  • Modified DirDeletingTask to accept a snapshotId in its constructor (line 206).
  • Added getSetSnapshotRequestUpdatingExclusiveSize helper method (lines 216-225).
  • Added processDeletedDirsForStore method to handle processing for a specific store (AOS or snapshot) using multiple threads (lines 233-289).
  • Added processDeletedDirectories helper method to perform the core directory processing logic, including using filters and updating exclusive size maps (lines 291-375).
  • Removed the previousSnapshotHasDir method (lines 284-341 in old code).
  • Removed the getPendingDeletedDirInfo method (lines 344-347 in old code).
  • Updated the call method to use the new processDeletedDirsForStore method and handle snapshot-specific logic (lines 378-427).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java
  • Removed InterruptedException from the throws clause of processDeletedKeysForStore (line 207).
  • Updated log message in call for deep cleaned snapshots to include snapshot name (lines 326-327).
  • Removed InterruptedException from the throws clause of call (line 348).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java
  • Added import for OMException (line 36).
  • Modified initializePreviousSnapshotsFromChain to use ozoneManager.getBucketManager().getBucketInfo and handle VOLUME_NOT_FOUND or BUCKET_NOT_FOUND exceptions by setting bucketInfo and volumeId to null (lines 175-185).
  • Modified apply method to return true immediately if bucketInfo is null (indicating the volume/bucket was deleted) (line 201).
• hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java
  • Added imports for BucketManager and OMException (lines 51, 58).
  • Added parameterized tests testReclaimableFilterSnapshotChainInitializationWithInvalidVolume and testReclaimableFilterSnapshotChainInitializationWithInvalidBucket to test filter initialization with non-existent volumes/buckets (lines 133-161).
  • Modified getLastSnapshotInfos to use getOrDefault to handle cases where the bucket key might not exist in the map (line 246).
  • Modified mockOzoneManager to mock BucketManager and provide custom logic for getBucketInfo to simulate volume/bucket not found exceptions (lines 164-185).
• hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java
  • Updated testReclaimableDirFilter to use getOzoneManager().getBucketManager().getBucketInfo instead of getOzoneManager().getBucketInfo (line 75).
• hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java
  • Updated testReclaimableKeyFilter to use getOzoneManager().getBucketManager().getBucketInfo instead of getOzoneManager().getBucketInfo (line 104).
• hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java
  • Updated testReclaimableRenameEntryFilter to use getOzoneManager().getBucketManager().getBucketInfo instead of getOzoneManager().getBucketInfo (line 83).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/BlockExistenceVerifier.java
  • Added setId(datanode.getID()) when building the pipeline (line 59).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/ContainerStateVerifier.java
  • Added a new file ContainerStateVerifier.java implementing ReplicaVerifier to check container and replica states.
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/ReplicasVerify.java
  • Added --container-cache-size command-line option (lines 70-76).
  • Added logic to include ContainerStateVerifier in the list of verifiers if the --container-state option is used (lines 91-93).
  • Added doExecuteReplicaState boolean field to the Verification inner class, controlled by the --container-state option (lines 236-241).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/chunk/ChunkKeyHandler.java
  • Changed the JSON key from ecIndex to replicaIndex when reporting EC chunk information (line 199).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[Copilot]

Pull Request Overview

This PR refactors the directory deletion logic to unify snapshot and AOS directory cleanup under the new DirectoryDeletingService using ReclaimableDirectoryFilter and ReclaimableKeyFilter, and removes the old SnapshotDirectoryCleaningService.

• Introduce dirDeletingService in OM bootstrap and remove snapshotDirectoryCleaningService.
• Update KeyManager API and implementation to use generic WithParentObjectId and Table.KeyValue alias.
• Adjust integration tests to use new service, filters, and updated trash policy class.

Reviewed Changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java	Added getDirDeletingService() to lock list; removed old snapshot directory service
ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java	Replaced SnapshotDirectoryCleaningService with DirectoryDeletingService, updated iterator types to KeyValue
ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManager.java	Updated API signatures for directory/file sub-path retrieval with filter parameter
integration-test/.../TestSnapshotDirectoryCleaningService.java	Switched to DirectoryDeletingService and added snapshot chain waiting logic
integration-test/.../TestDirectoryDeletingServiceWithFSO.java	Updated service instantiation to include new filter arguments and replaced assertions with waitFor
integration-test/.../TestRandomKeyGenerator.java	Added --bucket-layout option and exposed bucket map methods for testing
integration-test/.../TestOzoneShellHA.java	Updated trash policy import and documentation to use OzoneTrashPolicy
common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java	Marked snapshot directory service configs as @Deprecated
common/src/main/resources/ozone-default.xml	Updated default trash policy and deprecated tags for snapshot configs
server-scm/.../ContainerBalancer.java	Added new validation rule for move timeouts with detailed error message
server-scm/.../TestContainerBalancer.java	Enhanced test to validate new exception message and assert stopped status

Comments suppressed due to low confidence (2)

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManager.java:310

• The Javadoc above getPendingDeletionSubFiles no longer mentions the new filter parameter. Update the comment to describe what the filter function does and when it is applied.

DeleteKeysResult getPendingDeletionSubFiles(long volumeId, long bucketId, OmKeyInfo parentInfo, CheckedFunction<Table.KeyValue<String, OmKeyInfo>, Boolean, IOException> filter, long remainingBufLimit) throws IOException;

hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDirectoryCleaningService.java:247

• Missing import for SnapshotUtils causes a compilation error. Add import org.apache.hadoop.ozone.om.snapshot.SnapshotUtils; at the top of the test file.

SnapshotInfo nextSnapshot = SnapshotUtils.getNextSnapshot(cluster.getOzoneManager(), snapshotChainManager,

[Copilot]

[nitpick] The use of Duration.ofMillis(datanodeOffset).toMillis() is redundant since datanodeOffset is already in milliseconds. You can simplify the error message formatting by using datanodeOffset directly.

Suggested change

	Duration.ofMillis(datanodeOffset).toMillis());
	datanodeOffset);

[Copilot]

[nitpick] This assertion only checks for a substring in the exception message. Consider asserting the full message or using a more expressive assertion helper to guard against unintended changes in the text.

Suggested change

	assertTrue(ex.getMessage().contains("should be greater than or equal to 540000ms or 9 minutes"),
	"Exception message should contain 'should be greater than or equal to 540000ms or 9 minutes'");
	String expectedMessage = "(hdds.container.balancer.move.timeout - hdds.container.balancer.move.replication.timeout " +
	"- hdds.scm.replication.event.timeout.datanode.offset) should be greater than or equal to 9 minutes.";
	assertEquals(expectedMessage, ex.getMessage(), "Exception message should match the expected message.");

[Copilot]

[nitpick] The magic number 7 for minimum run count is unclear. Extract it into a named constant or add a comment explaining why at least 7 runs are expected to improve readability.

Suggested change

	GenericTestUtils.waitFor(() -> dirDeletingService.getRunCount().get() - preRunCount >= 7, 1000, 100000);
	GenericTestUtils.waitFor(() -> dirDeletingService.getRunCount().get() - preRunCount >= MINIMUM_RUN_COUNT_FOR_SAFE_CI, 1000, 100000);

[gemini-code-assist]

Code Review

This pull request effectively refactors the DirectoryDeletingService to unify Active Object Store (AOS) and snapshot directory deletion logic using ReclaimableDirectoryFilter and ReclaimableKeyFilter. This is a significant improvement for code simplification and maintainability. The changes are comprehensive, touching configuration, core logic, documentation, and tests.

The introduction of parallel processing per store in DirectoryDeletingService and the new ContainerStateVerifier are valuable additions. The deprecation of old configurations and services is also handled consistently.

I've identified a few areas for potential improvement, mainly around code clarity, constant usage, and test timeouts. Addressing these would further enhance the quality of this contribution.

Summary of Findings

• Refactoring and Unification: The core refactoring of DirectoryDeletingService to handle both AOS and snapshot directory deletions is a significant improvement, centralizing logic and reducing redundancy.
• Configuration and Service Lifecycle: Old snapshot directory cleaning services and configurations are correctly deprecated and removed, aligning with the refactoring.
• Clarity and Maintainability: Some areas, particularly around complex logic in DirectoryDeletingService and the use of magic numbers in ContainerBalancer, could benefit from enhanced comments or constants for better long-term maintainability.
• Error Handling: The error handling in asynchronous operations within DirectoryDeletingService could be more specific to avoid masking critical system errors.
• Testing: Test suites have been broadly updated, which is good. Some test timeouts appear excessively long and could be reviewed.

Merge Readiness

This pull request introduces a substantial and beneficial refactoring by unifying the directory deletion logic. The changes are extensive and appear to be well-covered by test updates.

I've made a few suggestions of medium severity primarily aimed at improving code clarity, maintainability (e.g., using constants instead of magic numbers, clarifying constructor parameters), and refining error handling in asynchronous parts of the DirectoryDeletingService. Additionally, a very long timeout in one of the integration tests might warrant a review.

While these points are not critical blockers, addressing them would further strengthen the codebase. As I am an AI assistant, I cannot approve pull requests. This PR should be reviewed and approved by other maintainers. I recommend addressing the suggested changes before merging.

[gemini-code-assist]

The new validation logic for move timeouts (lines 469-470) uses a hardcoded duration of 9 minutes and its millisecond equivalent 540000ms.

    if ((conf.getMoveTimeout().toMillis() - conf.getMoveReplicationTimeout().toMillis() - datanodeOffset)
        < Duration.ofMinutes(9).toMillis()) { // 9 minutes
      String msg = String.format("(hdds.container.balancer.move.timeout (%sms) - " +
              "hdds.container.balancer.move.replication.timeout (%sms) - " +
              "hdds.scm.replication.event.timeout.datanode.offset (%sms)) " +
              "should be greater than or equal to 540000ms or 9 minutes.", // 540000ms
          conf.getMoveTimeout().toMillis(),
          conf.getMoveReplicationTimeout().toMillis(),
          Duration.ofMillis(datanodeOffset).toMillis());

Could these values (9 minutes and 540000ms) be defined as named constants for better readability and maintainability? For example:

private static final Duration MIN_MOVE_TIMEOUT_DIFFERENCE = Duration.ofMinutes(9);
// ...
    if ((conf.getMoveTimeout().toMillis() - conf.getMoveReplicationTimeout().toMillis() - datanodeOffset)
        < MIN_MOVE_TIMEOUT_DIFFERENCE.toMillis()) {
      String msg = String.format("... should be greater than or equal to %sms or %s minutes.",
          MIN_MOVE_TIMEOUT_DIFFERENCE.toMillis(),
          MIN_MOVE_TIMEOUT_DIFFERENCE.toMinutes());
      // ...
    }

Additionally, on line 477, Duration.ofMillis(datanodeOffset).toMillis() is used. If datanodeOffset is already in milliseconds (as obtained from ozoneConfiguration.getTimeDuration with TimeUnit.MILLISECONDS), this conversion seems redundant. Could datanodeOffset be used directly in the String.format call here?

[gemini-code-assist]

The timeout value 100000000 (milliseconds, which is ~27.7 hours) for GenericTestUtils.waitFor seems excessively large for an integration test.

      }, 2000, 100000000);

Is such a long timeout necessary here? Could this potentially mask underlying issues or lead to very long test execution times if the condition is never met? Consider reducing this to a more typical timeout for integration tests (e.g., a few minutes) unless this specific scenario truly requires such an extended wait.

[gemini-code-assist]

The DirectoryDeletingService constructor now includes a deepCleanSnapshots boolean parameter.

  public DirectoryDeletingService(long interval, TimeUnit unit,
      long serviceTimeout, OzoneManager ozoneManager,
      OzoneConfiguration configuration, int dirDeletingServiceCorePoolSize, boolean deepCleanSnapshots) {
    super(DirectoryDeletingService.class.getSimpleName(), interval, unit,
        dirDeletingServiceCorePoolSize, serviceTimeout, ozoneManager, null);
    // ...
    this.deepCleanSnapshots = deepCleanSnapshots;
  }

This flag appears to control whether the service processes directories within snapshots (as seen in getTasks() method, lines 160-172). Could a Javadoc comment be added to the constructor or the class to clarify the role of deepCleanSnapshots and how it influences the service's behavior? This would enhance understanding for future maintenance.

[gemini-code-assist]

In DirDeletingTask#processDeletedDirsForStore, the CompletableFuture.supplyAsync lambda catches a generic Throwable e:

            } catch (Throwable e) { // Catches Throwable
              return false;
            }

While this ensures the future completes and returns false on error, catching Throwable can hide critical errors like OutOfMemoryError or other Errors that might indicate more severe system problems.

Would it be more appropriate to catch specific, expected exceptions (e.g., IOException, RuntimeException) that processDeletedDirectories might throw, and let other Errors propagate or be handled differently? This could aid in diagnosing more serious issues if they occur.

[gemini-code-assist]

The logic for handling OMException (specifically VOLUME_NOT_FOUND or BUCKET_NOT_FOUND) within initializePreviousSnapshotsFromChain is as follows:

      try {
        bucketInfo = ozoneManager.getBucketManager().getBucketInfo(volume, bucket);
        volumeId = ozoneManager.getMetadataManager().getVolumeId(volume);
      } catch (OMException e) {
        // If Volume or bucket has been deleted then all keys should be reclaimable as no snapshots would exist.
        if (OMException.ResultCodes.VOLUME_NOT_FOUND == e.getResult() ||
            OMException.ResultCodes.BUCKET_NOT_FOUND == e.getResult()) {
          bucketInfo = null;
          volumeId = null;
          return; // Exits initialization early
        }
        throw e;
      }

When a volume/bucket is not found, bucketInfo and volumeId are set to null, and the method returns. Subsequently, in the apply() method (line 201), bucketInfo == null is correctly treated as a condition for reclaimability.

However, if initializePreviousSnapshotsFromChain returns early due to VOLUME_NOT_FOUND or BUCKET_NOT_FOUND, the previousOmSnapshots and previousSnapshotInfos lists (which are cleared and repopulated in this method) might not be fully populated as intended for the numberOfPreviousSnapshotsFromChain.

Does the isReclaimable(keyValue) method, which might rely on these lists, correctly handle this state where bucketInfo is null but the snapshot-related lists might be empty or only partially initialized from a prior successful call for a different key?

A brief comment clarifying this interaction or ensuring these lists are reset to a consistent state (e.g., fully cleared if bucketInfo becomes null during initialization) could be beneficial for maintainability.