HDDS-13442. Replace OMKeyInfo with light-weight ReconBasicOmKeyInfo under OmTableHandlers #8809
Conversation
…nder OmTableHandlers
devabhishekpal
left a comment
devabhishekpal
left a comment
There was a problem hiding this comment.
Thanks for this change @tanvipenumudy
LGTM, +1
@devmadhuu @ArafatKhan2198 could you take a look as well?
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
Thanks, Tanvi! Most of the changes look great. Could you please add a brief JavaDoc comment at the beginning?
Additionally, the code would benefit from more comprehensive documentation explaining the relationship between OmKeyInfo and ReconBasicOmKeyInfo. Please add class-level JavaDoc to clarify this.
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
@tanvipenumudy Thanks for the patch, one comment. Pls check.
| return builder.build(); | ||
| } | ||
|
|
||
| public static ReconBasicOmKeyInfo getFromProtobuf(OzoneManagerProtocolProtos.KeyInfo keyInfoProto) { |
There was a problem hiding this comment.
Any reason this method is duplicated ?
There was a problem hiding this comment.
Yes, the existing method takes OzoneManagerProtocolProtos.KeyInfoProtoLight as an argument while the new overloaded method takes OzoneManagerProtocolProtos.KeyInfo as an argument!
There was a problem hiding this comment.
I see that since PartKeyInfo is using KeyInfo proto and even though you are not using keylocations and other fields, these fields will still be decoded from byte buffer to proto fields. Is this Ok ?
There was a problem hiding this comment.
That's correct, the PR only addresses one half of the issue with performance.
PartKeyInfo is using KeyInfo proto and even though you are not using keylocations and other fields, these fields will still be decoded from byte buffer to proto fields
To optimize this further, we may need a new implementation of PartKeyInfoMap that uses an updated version of Iterable based on the KeyInfoProtoLight proto object instead of KeyInfo. However, this could require significant changes, not sure if there's a good way to handle this!
cc: @sumitagrawl
There was a problem hiding this comment.
As discussed offline, created the ticket: HDDS-13490 for addressing the remaining portion of the optimization!
Thank you @ArafatKhan2198 for the review! I believe this is already addressed in PR: #8699, that introduces the |
priyeshkaratha
left a comment
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thanks for this change @tanvipenumudy. Changes looks good for me.
|
Thank you @devabhishekpal, @devmadhuu, @priyeshkaratha and @ArafatKhan2198 for reviewing the patch! |
…nder OmTableHandlers (apache#8809)
…nder OmTableHandlers (apache#8809)
6 participants
What changes were proposed in this pull request?
Recon OmTableHandlers currently rely on the full KeyInfo proto for key metadata. However, most of the fields in OMKeyInfo, such as KeyLocationList, FileEncryptionInfoProto, OzoneAclInfo, and FileChecksumProto, are not required and contribute to unnecessary serialization and deserialization overhead.
This inefficiency becomes significant for larger keys or multipart upload (MPU) keys, where the KeyLocationList can be very large. As a result, this can cause performance degradation for clients querying this API.
To address this, OMKeyInfo objects should be replaced with a lighter weight ReconBasicOmKeyInfo object (based on the KeyInfoProtoLight proto) introduced in PR #8699. This lightweight version includes only the necessary fields required for event handling.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13442
How was this patch tested?