Skip to content

HDDS-14957. Separate OpenKeyInfo to differentiate KeyInfo#10181

Open
YutaLin wants to merge 5 commits intoapache:masterfrom
YutaLin:HDDS-14957_Seperate_OpenKeyInfo
Open

HDDS-14957. Separate OpenKeyInfo to differentiate KeyInfo#10181
YutaLin wants to merge 5 commits intoapache:masterfrom
YutaLin:HDDS-14957_Seperate_OpenKeyInfo

Conversation

@YutaLin
Copy link
Copy Markdown
Contributor

@YutaLin YutaLin commented May 3, 2026

What changes were proposed in this pull request?

Introduce new codec for OmKeyInfo to filter fields only for OpenKeyTable

What is the link to the Apache JIRA

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

How was this patch tested?

CI Actions(https://github.com/YutaLin/ozone/actions/runs/25272906337)

@YutaLin
Copy link
Copy Markdown
Contributor Author

YutaLin commented May 3, 2026

Hi @peterxcli, @ivandika3
Could you help me review this? thanks

Copy link
Copy Markdown
Member

@peterxcli peterxcli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @YutaLin for this patch. Code overall looks good, could you help me to see if we could introduce new class om open key info and repeated om key info with minimal copying overheard? As I think that would be much cleaner. Appreciate!

@YutaLin
Copy link
Copy Markdown
Contributor Author

YutaLin commented May 4, 2026

Hi @peterxcli, thanks for review!
OmOpenKeyInfo is cleaner, but the benefit may not justify the effort. It would require changes across many files, and the current solution already improves the problem.

Comment on lines +883 to +889
if (isOpenKey) {
if (expectedDataGeneration != null) {
kb.setExpectedDataGeneration(expectedDataGeneration);
}
if (expectedETag != null) {
kb.setExpectedETag(expectedETag);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My ultimate goal is to remove this, like just make the OmKeyInfo do not accept these field as member, and that's why I purpose we have to have a named Codec class with the OmKeyInfo proto msg for each scenario(eg. key table, open key table, file table)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely can just merge current change, and leave this as followup for more reasoning.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @szetszwo for any thoughts when convenient.

Copy link
Copy Markdown
Member

@peterxcli peterxcli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let’s wait until #10182 is merged.

@peterxcli peterxcli requested review from peterxcli and szetszwo May 4, 2026 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants