HDDS-13170. Reclaimable filter should always reclaim entries when buc… #568
Conversation
…kets and volumes have already been deleted Change-Id: I16dc9d8f00686320b4e98fa5691420294a7f1e2f
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @jojochuang, I'm Gemini Code Assist1! 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 here, providing a summary of this pull request. This PR addresses an issue in the ReclaimableFilter within Ozone's snapshot mechanism. The filter is responsible for determining which entries (like keys or directories) are no longer needed and can be reclaimed during snapshot diff processing. Previously, if the volume or bucket associated with an entry had been deleted, the filter would encounter an error when trying to retrieve information about the volume or bucket. This PR modifies the filter to gracefully handle cases where the volume or bucket is not found, ensuring that entries belonging to deleted volumes or buckets are correctly identified as reclaimable. This is achieved by catching VOLUME_NOT_FOUND and BUCKET_NOT_FOUND exceptions and marking the entry as reclaimable if the parent container (volume/bucket) is gone.
Highlights
- Bug Fix: Fixes a bug in the
ReclaimableFilterwhere it would fail if the volume or bucket associated with an entry had been deleted. - Error Handling: Adds specific error handling for
OMExceptionwithVOLUME_NOT_FOUNDorBUCKET_NOT_FOUNDresult codes during the initialization of the snapshot chain for a given volume/bucket. - Reclamation Logic: Ensures that entries are considered reclaimable if their parent volume or bucket no longer exists, as there would be no snapshots to retain them.
- Test Coverage: Adds new test cases to verify the correct behavior of the
ReclaimableFilterwhen volumes or buckets are not found.
Changelog
Click here to see the changelog
- hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java
- Imported
OMException. - Wrapped the calls to
ozoneManager.getBucketManager().getBucketInfoandozoneManager.getMetadataManager().getVolumeIdininitializePreviousSnapshotsFromChainwithin a try-catch block to handleIOException. - Added specific handling within the catch block for
OMExceptionwithVOLUME_NOT_FOUNDorBUCKET_NOT_FOUNDresult codes (lines 177-185). If caught,bucketInfoandvolumeIdare set to null, and the method returns, indicating that the parent container is gone. - Modified the
applymethod to check ifbucketInfois null (line 201). If it is, the entry is immediately considered reclaimable (isReclaimable = (bucketInfo == null) || isReclaimable(keyValue)).
- Imported
- hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java
- Imported
BucketManagerandOMException. - Updated
mockOzoneManagerto mockBucketManagerand return it viaozoneManager.getBucketManager()(lines 164-166). - Replaced the direct mocking of
ozoneManager.getBucketInfowith anthenAnswerimplementation forbucketManager.getBucketInfo(lines 173-185). This allows simulatingOMExceptionfor volumes or buckets not present in the test setup'svolumesorbucketslists. - Updated
getLastSnapshotInfosto usegetOrDefaultwhen accessing the snapshot infos map (line 246), providing an empty list if the volume/bucket key is not found, preventing NullPointerExceptions in test scenarios involving non-existent containers.
- Imported
- hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java
- Updated the call to get
OmBucketInfofromgetOzoneManager().getBucketInfotogetOzoneManager().getBucketManager().getBucketInfo(line 75).
- Updated the call to get
- hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java
- Added new parameterized test methods
testReclaimableFilterSnapshotChainInitializationWithInvalidVolume(lines 135-146) andtestReclaimableFilterSnapshotChainInitializationWithInvalidBucket(lines 148-161) to specifically test the filter's behavior when the volume or bucket is not found.
- Added new parameterized test methods
- hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java
- Updated the call to get
OmBucketInfofromgetOzoneManager().getBucketInfotogetOzoneManager().getBucketManager().getBucketInfo(line 104).
- Updated the call to get
- hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java
- Updated the call to get
OmBucketInfofromgetOzoneManager().getBucketInfotogetOzoneManager().getBucketManager().getBucketInfo(line 83).
- Updated the call to get
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.
Data flows like a stream,
Snapshots capture a dream.
When buckets are gone,
Reclaim moves right on,
Cleaning up, it would seem.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to ensure that the ReclaimableFilter correctly identifies entries as reclaimable when their associated buckets or volumes have been deleted. The core logic change involves handling OMException (specifically VOLUME_NOT_FOUND or BUCKET_NOT_FOUND) during the initialization of bucket and volume information. If such an exception occurs, bucketInfo is set to null, and the filter's apply method now considers bucketInfo == null as a sufficient condition for an entry to be reclaimable.
The changes to the test classes, particularly AbstractReclaimableFilterTest and the new tests in TestReclaimableFilter, are crucial for verifying this new behavior. They correctly mock the scenarios where volumes or buckets are not found.
Overall, the approach seems sound and directly addresses the issue described by the PR title. There's one minor point in the test code that could be improved for clarity.
Please also consider filling out the pull request description template with details about the changes, rationale, JIRA link, and testing strategy. This greatly helps reviewers and future maintainers understand the context of the PR.
Summary of Findings
- Test Clarity: Mocked exception messages in
AbstractReclaimableFilterTest.javaforVOLUME_NOT_FOUNDandBUCKET_NOT_FOUNDshould be updated to accurately reflect a "not found" scenario rather than an "already exists" scenario. - PR Description: The pull request description is currently a template. It would be beneficial to fill it out with details about the changes, rationale, JIRA link, and how the patch was tested.
Merge Readiness
The core logic changes in this PR appear correct and effectively address the intended issue. The accompanying test modifications and additions are also good.
There is one medium-severity issue identified in the test code regarding misleading exception messages in a mock setup. It's recommended to address this for better test clarity.
Additionally, please consider completing the pull request description template to provide more context for this change.
I am not authorized to approve pull requests. Once the suggested change is made and the PR description is updated, this PR should be in a good state for further review and merging by authorized maintainers.
| if (!volumes.contains(volume)) { | ||
| throw new OMException("Volume " + volume + " already exists", OMException.ResultCodes.VOLUME_NOT_FOUND); | ||
| } | ||
| if (!buckets.contains(bucket)) { | ||
| throw new OMException("Bucket " + bucket + " already exists", OMException.ResultCodes.BUCKET_NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
The exception messages for VOLUME_NOT_FOUND and BUCKET_NOT_FOUND in the mock setup seem to be misleading. For example, for VOLUME_NOT_FOUND, the message is "Volume " + volume + " already exists". Shouldn't this be more like "Volume " + volume + " not found" to accurately reflect the exception code being thrown?
This could cause confusion during debugging if a test fails due to these specific exceptions.
…kets and volumes have already been deleted
Change-Id: I16dc9d8f00686320b4e98fa5691420294a7f1e2f
What changes were proposed in this pull request?
Provide a one-liner summary of the changes in the PR Title field above.
It should be in the form of
HDDS-1234. Short summary of the change.Please describe your PR in detail:
perspective not just for the reviewer.
the Jira's description if the jira is well defined.
issue investigation, github discussion, etc.
Examples of well-written pull requests:
What is the link to the Apache JIRA
Please create an issue in ASF JIRA before opening a pull request, and you need to set the title of the pull
request which starts with the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
(Please replace this section with the link to the Apache JIRA)
How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests, workflow run on the fork git repo.)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this.)