ivandika3 commented on code in PR #11289:
URL: https://github.com/apache/ozone/pull/11289#discussion_r4068053951
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFollowerReadFailoverProxyProvider.java:
##########
@@ -219,69 +237,47 @@ private synchronized OMProxyInfo<OzoneManagerProtocolPB>
changeProxy(OMProxyInfo
}
/**
- * An InvocationHandler to handle incoming requests. This class's invoke
- * method contains the primary logic for redirecting to followers.
+ * A protocol implementation that redirects incoming requests to followers.
* <p>
* If follower reads are enabled, attempt to send read operations to the
* current proxy which can be either a leader or follower. If the current
* proxy's OM node fails, adjust the current proxy and return on the next
one.
* <p>
* Write requests are always forwarded to the leader.
*/
- private class FollowerReadInvocationHandler implements RpcInvocationHandler {
+ private class FollowerReadProxy implements OzoneManagerProtocolPB, RpcProxy {
@Override
- public Object invoke(Object proxy, final Method method, final Object[]
args)
- throws Throwable {
+ public OMResponse submitRequest(RpcController controller, OMRequest
omRequest) throws ServiceException {
lastProxy = null;
- if (method.getDeclaringClass() == Object.class) {
- // If the method is not a OzoneManagerProtocolPB method (e.g.
Object#toString()),
- // we should invoke the method on the current proxy
- return method.invoke(this, args);
- }
- OMRequest omRequest = parseOMRequest(args);
-
- // Apply default consistency hint once, before any routing decision.
- // 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);
Review 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`.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFollowerReadFailoverProxyProvider.java:
##########
@@ -148,34 +144,56 @@ public RetryPolicy getRetryPolicy(int maxFailovers) {
// for a few reasons
// 1. We want to ensure that the retry policy behavior remains the same
when we use the leader proxy
// (when follower read is disabled or using write request)
- // 2. The FollowerInvocationHandler is also written so that the thrown
exception is handled by the
+ // 2. The FollowerReadProxy is also written so that the thrown exception
is handled by the
// OMFailoverProxyProviderbase's RetryPolicy
return leaderProxy.getRetryPolicy(maxFailovers);
}
/**
- * Parse the OM request from the request args.
- *
- * @return parsed OM request.
+ * Create a client that applies the default consistency hint once, before
entering the retry loop.
*/
- private static OMRequest parseOMRequest(Object[] args) throws
ServiceException {
- String error = null;
- if (args == null) {
- error = "args == null";
- } else if (args.length < 2) {
- error = "args.length == " + args.length + " < 2";
- } else if (args[1] == null) {
- error = "args[1] == null";
- } else if (!(args[1] instanceof OMRequest)) {
- error = "Non-OMRequest: " + args[1].getClass();
+ public OzoneManagerProtocolPB newProxy(int maxFailovers) {
+ OzoneManagerProtocolPB retryProxy = (OzoneManagerProtocolPB)
RetryProxy.create(
+ OzoneManagerProtocolPB.class, this, getRetryPolicy(maxFailovers));
+ return new ReadConsistencyProxy(retryProxy);
+ }
Review Comment:
Let's document the nesting of the proxies
My understanding is now it's
- `ReadConsistencyProxy`
- `RetryProxy`
- `HadoopRpcOMFollowerReadFailoverProxyProvider`
- `HadoopRpcOMFailoverProxyProvider`
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ipc_/RpcProxy.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ipc_;
+
+import java.io.Closeable;
+import org.apache.hadoop.ipc_.Client.ConnectionId;
+
+/**
+ * Connection and lifecycle access for RPC proxies, including concrete
protocol implementations.
+ */
+public interface RpcProxy extends Closeable {
Review 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`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
