HDDS-15511. Implement Ozone client API's for bucket tagging#10533
HDDS-15511. Implement Ozone client API's for bucket tagging#10533Gargi-jais11 wants to merge 1 commit into
Conversation
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
@Gargi-jais11 Thanks for the patch. Overall LGTM. Left some comments.
| /** | ||
| * S3-style bucketTags on the bucket. | ||
| */ | ||
| private final Map<String, String> bucketTags = new HashMap<>(); |
There was a problem hiding this comment.
Do we need this in-memory bucket tags? Similar to object tagging, I think we should use simply use getObjectTagging when we want to see the latest updated bucket tags (there might be other concurrent bucket concurrent bucket tags modification).
However, I also understand that OzoneBucket also contains in-memory stats that might be changed in OM DB.
There was a problem hiding this comment.
This is related to #10553, we can probably discuss it there.
| ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink( | ||
| Pair.of(volumeName, bucketName), this); |
There was a problem hiding this comment.
Not related to this patch (can be addressed in future patch), I'm just thinking that for preExecute, we can attach the the bucket object ID in preExecute and let the validateAndUpdateCache check that the bucket object ID in the preExecute is the same as the bucket object ID in validateAndUpdateCache, if it doesn't we let the validateAndUpdateCache try to query the bucket link again. We might also need to do the same for permission check. This way if there is a bucket deletion and creation in the same between the preExecute and validateAndUpdateCache, we will still check the latest bucket changes.
There was a problem hiding this comment.
After reviewing @errose28 previous comments at jira, I’m not sure if implementing this is necessary.
However, I was thinking that we could potentially adopt this logic for conditional requests, primarily for two reasons:
- Fail-fast: It reduces the number of unnecessary requests that need to be sent to Ratis
- Minimal Overhead: Although incorporating this logic adds an extra validation step, it’s just a string comparison which is extremely fast. Furthermore, once the metadata is fetched from RocksDB during the initial read, it becomes warm data in the cache, ensuring that subsequent reads will result in cache hits and execute very quickly.
@ivandika3 What are your thoughts on this?
There was a problem hiding this comment.
Thanks @chungen0126 for checking this.
I think the scenario mentioned by @errose28 is a case when the object is not overwritten, which I think is not a major problem. However, a problem is if the object is overwritten (with different object ID) and there were some pending transactions that was acting on the previous overwritten object. I think this is unlikely since it requires openKey and commitKey almost instantaneously, but it is possible.
However, I was thinking that we could potentially adopt this logic for conditional requests, primarily for two reasons:
Yes, I think @peterxcli has propose similar logic as well. For now I think we can use this for conditional requests.
There was a problem hiding this comment.
Thank you @ivandika3 for clarifying the scenario. I will need to think about this more deeply later on.
Yes, I think @peterxcli has propose similar logic as well. For now I think we can use this for conditional requests.
Thanks for reminding me. I just saw it.
There was a problem hiding this comment.
Yes, I was subconsciously reminded of this, let's review #10183 in that case.
There was a problem hiding this comment.
This discussion reminds me of this: https://authzed.com/blog/new-enemies
4 participants
What changes were proposed in this pull request?
getBucketTagging/putBucketTagging/deleteBucketTaggingon Ozone client with version gating.ClientProtocol.java / OzoneManagerProtocol.java— interface methodsOzoneManagerProtocolClientSideTranslatorPB.java— PB translationRpcClient.java— getBucketTagging, putBucketTagging, deleteBucketTagging with S3_BUCKET_TAGGING_API version gateOzoneBucket.java— public client APIOzoneRpcClientTests.javaintegration tests:- testPutBucketTagging (overwrite semantics)
- testGetBucketTagging
- testDeleteBucketTagging
- Parameterized across bucket layouts (LEGACY, FILE_SYSTEM_OPTIMIZED, OBJECT_STORE)
If we could introduce a base class for bucket tagging, or make delete request to derive put request.
the duplication between
S3PutBucketTaggingRequestand S3DeleteBucketTaggingRequestDiscussion link
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15511
How was this patch tested?
Added unit tests for 'OzoneRpcClientTests`.