Skip to content

HDDS-15205. Cut WritableRPCEngine#10213

Open
adoroszlai wants to merge 2 commits intoapache:masterfrom
adoroszlai:HDDS-15205
Open

HDDS-15205. Cut WritableRPCEngine#10213
adoroszlai wants to merge 2 commits intoapache:masterfrom
adoroszlai:HDDS-15205

Conversation

@adoroszlai
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Port from Hadoop: HADOOP-19864. Cut WritableRPCEngine.

Ozone uses ProtobufRpcEngine, never used WritableRPCEngine.

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

How was this patch tested?

https://github.com/adoroszlai/ozone/actions/runs/25539302248

@adoroszlai adoroszlai self-assigned this May 8, 2026
Copy link
Copy Markdown
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

LGTM +1. Same as HADOOP-19764 but without additional comments.

@adoroszlai
Copy link
Copy Markdown
Contributor Author

Thanks @ivandika3 for the review. I have noticed a follow-up PR (apache/hadoop#8470), included in another commit here.

@ivandika3
Copy link
Copy Markdown
Contributor

ivandika3 commented May 8, 2026

@adoroszlai The implementation looks good and there is already a +1 so I think it's ok to include in this patch. I'm still +1.

@ivandika3
Copy link
Copy Markdown
Contributor

ivandika3 commented May 8, 2026

@adoroszlai I think we should include the test testUnregisteredRpcKindRejectedWithoutDeserialization.

In the future Ozone might decide to change the Hadoop RPC logic to suite Ozone's need so we need test to catch regressions.

@adoroszlai
Copy link
Copy Markdown
Contributor Author

I think we should include the test testUnregisteredRpcKindRejectedWithoutDeserialization.

Unit tests for Hadoop RPC are not included in Ozone, because RPC was copied from Hadoop 3.2, the last version without ProtobufRpcEngine2, and tests in that version use JUnit4, which we no longer support.

We can attempt to copy and migrate tests in a follow-up.

@ivandika3
Copy link
Copy Markdown
Contributor

@adoroszlai Got it, thanks for the info.

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