HDDS-16161. Prevent failed OM requests from changing bucket usage - #11216
Open
chihsuan wants to merge 15 commits into
Open
HDDS-16161. Prevent failed OM requests from changing bucket usage#11216chihsuan wants to merge 15 commits into
chihsuan wants to merge 15 commits into
Conversation
… parent directories
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR prevents failed Ozone Manager (OM) requests from leaking in-memory bucket usage updates by switching handlers to a read-copy-modify-publish flow, publishing updated bucket info only when the response will be persisted.
Changes:
- Added
getBucketInfoForUpdate()to return a copy for mutation and updated multiple request handlers to use it. - Published bucket usage updates to the bucket table cache only on successful/persisted paths (including partial deletes).
- Updated/added unit tests to validate that failures don’t mutate cached bucket usage and that repeated buckets accumulate into a single published update.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java | Introduces getBucketInfoForUpdate() and clarifies read-only vs mutating access. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java | Uses copy-for-update and publishes bucket row even on PARTIAL_DELETE. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java | Uses copy-for-update and publishes bucket cache entry on delete. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequestWithFSO.java | Same as above for FSO delete handler. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java | Uses copy-for-update and publishes bucket cache entry for missing-parent creation. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequestWithFSO.java | Same publishing pattern for FSO create. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java | Uses copy-for-update and publishes bucket cache entry on commit. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java | Same as above for FSO commit handler. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java | Uses copy-for-update and publishes bucket updates during purge. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java | Accumulates bucket deltas and publishes them after processing all purge paths. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java | Uses copy-for-update and publishes bucket cache entry for file create. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequestWithFSO.java | Same as above for FSO file create. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java | Uses copy-for-update and publishes bucket cache entry for directory create. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java | Publishes only after ACL-dependent steps succeed. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java | Switches to getBucketInfoForUpdate() to avoid leaking namespace charges on failure. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java | Uses copy-for-update and publishes bucket cache entry on success. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadAbortRequest.java | Uses copy-for-update and publishes bucket cache entry on success. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequestWithFSO.java | Uses copy-for-update and publishes bucket cache entry on success. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3ExpiredMultipartUploadsAbortRequest.java | Holds bucket write locks across request, accumulates per-bucket copies, publishes only after all succeed. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeysDeleteRequest.java | Adds test ensuring PARTIAL_DELETE publishes bucket usage to cache+DB. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeysDeleteRequestWithFSO.java | Adjusts FSO expectations and stubs default replication needed for deleted-dir writes. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyPurgeRequestAndResponse.java | Adds test verifying purge deltas reach cache and DB and accumulate correctly. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java | Adds tests for namespace charges reaching cache+DB and for non-replacement under key-path locking. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java | Adds test ensuring failed commit doesn’t leak bucket usage. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequestWithFSO.java | Adds test ensuring failed dir create doesn’t leak namespace quota. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3ExpiredMultipartUploadsAbortRequest.java | Adds tests for multi-bucket failure isolation and repeated-bucket accumulation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Extend the existing success tests for OMKeyDeleteRequest, S3MultipartUploadAbortRequest and S3InitiateMultipartUploadRequestWithFSO so that dropping the addCacheEntry publication fails them. Verified by removing each publication and re-running the tests.
The commit test asserted usedBytes, which is only charged after the injected failure point, so that half was vacuous. The partial delete test asserted a negative usedNamespace because the fixture never charged the bucket for the keys it added.
getBucketInfoForUpdate stated the bucket write lock as an unconditional rule, but a caller under key path locking holds only the read lock and must not publish. The expired MPU abort request now also writes the multipart info and bucket table caches.
# Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
OM request handlers can modify bucket usage directly in the cache before the request finishes. If a later step fails, the changed usage remains in memory even though it is not persisted.
The bucket table is cached in full, so modifying that object in place is also how the cache gets updated. Copying it is only half a fix: without publishing the copy, successful requests would stop recording usage.
This patch changes the update flow to read a copy → modify it → publish it when the changes will be persisted:
getBucketInfoForUpdate()and use it in key, directory, purge, and S3 multipart handlers that update bucket usage.The main review points are whether each handler publishes the copy at the correct point and holds the bucket write lock throughout the read-modify-publish sequence.
Audit
The audit covered 24 calls to
getBucketInfo()across 22 request classes: one already fixed by HDDS-16117, four read-only, and 17 updated here. Five of the 17 can already fail after changing usage; the rest follow the same pattern but have no reachable failure yet.OMKeyCreateRequestis the one exception to the flow above. It publishes only when it changes parent directories, because key path locking holds a bucket read lock and creates no parents.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16161
How was this patch tested?
Generated-by: Claude Code (Opus 5)