Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
These cross-cutting typed dispatch, retry, and RPC lifecycle changes warrant final human review.
Review effort: Lite
Findings: None
What changed in this PR
Replaces reflective OM follower routing with typed protocol dispatch while preserving retries, consistency hints, connection lookup, and proxy lifecycle behavior.
Changes:
- Adds typed follower-read and consistency proxies.
- Introduces shared RPC proxy lifecycle support.
- Expands routing, retry, interruption, and lifecycle tests.
| File | Description |
|---|---|
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/ha/TestHadoopRpcOMFollowerReadFailoverProxyProvider.java |
Tests routing, hints, interruptions, and lifecycle |
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolPB.java |
Delegates follower proxy creation |
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFollowerReadFailoverProxyProvider.java |
Implements typed routing and retry wrapping |
hadoop-hdds/common/src/test/java/org/apache/hadoop/io_/retry/TestRetryProxy.java |
Tests concrete proxy integration |
hadoop-hdds/common/src/main/java/org/apache/hadoop/ipc_/RpcProxy.java |
Adds the shared RPC proxy contract |
hadoop-hdds/common/src/main/java/org/apache/hadoop/ipc_/RpcInvocationHandler.java |
Reuses the shared proxy contract |
hadoop-hdds/common/src/main/java/org/apache/hadoop/ipc_/RPC.java |
Supports connection lookup for concrete proxies |
hadoop-hdds/common/src/main/java/org/apache/hadoop/io_/retry/RetryInvocationHandler.java |
Recognizes concrete RPC proxies |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
peterxcli
changed the title
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thanks @peterxcli for the patch. Overall looks good. Left some comments.
| /** | ||
| * Connection and lifecycle access for RPC proxies, including concrete protocol implementations. | ||
| */ | ||
| public interface RpcProxy extends Closeable { |
There was a problem hiding this comment.
So Closeable and getConnectionId is extracted to RpcProxy so that it can be used as a proxy along with the OzoneManagerProtocolPB instead of as part of InvocationHandler (which does not implement the OzoneManagerProtocolPB#submitRequest).
| // In the future, we will support per-request hints which allows client (e.g. S3 clients) | ||
| // to specify a custom request header (e.g. x-ozone-read-consistency) as a consistency hint | ||
| // for read requests. | ||
| omRequest = applyReadConsistency(omRequest); |
There was a problem hiding this comment.
Since we already have the ReadConsistencyProxy, do we still need the applyReadConsistency here? If this is not necessary, we can also put the applyReadConsistency under ReadConsistencyProxy.
| public OzoneManagerProtocolPB newProxy(int maxFailovers) { | ||
| OzoneManagerProtocolPB retryProxy = (OzoneManagerProtocolPB) RetryProxy.create( | ||
| OzoneManagerProtocolPB.class, this, getRetryPolicy(maxFailovers)); | ||
| return new ReadConsistencyProxy(retryProxy); | ||
| } |
There was a problem hiding this comment.
Let's document the multiple layer of proxy wrappings.
My understanding is now it's
ReadConsistencyProxyRetryProxyHadoopRpcOMFollowerReadFailoverProxyProviderHadoopRpcOMFailoverProxyProvider
What changes were proposed in this pull request?
Replace reflective OM follower routing with a concrete
OzoneManagerProtocolPBimplementation. DirectsubmitRequestcalls remove argument-array parsing and reflective dispatch. A sharedRpcProxycontract preserves connection lookup and lazy proxy creation, and consistency hints are applied before retries so fallback cannot change a request's hint.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16529
How was this patch tested?
33 focused follower-routing and retry tests passed, along with checkstyle and RAT. The full
mvn clean install -DskipTests -DskipReconbuild also passed.ci: https://github.com/peterxcli/ozone/actions/runs/35596424990