bharatviswa504 commented on a change in pull request #900:
URL: https://github.com/apache/hadoop-ozone/pull/900#discussion_r428354713
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
##########
@@ -216,23 +214,6 @@ public static void setScmId(String scmId){
TestOzoneRpcClientAbstract.scmId = scmId;
}
- /**
- * Test OM Proxy Provider.
- */
- @Test
- public void testOMClientProxyProvider() {
- OMFailoverProxyProvider omFailoverProxyProvider = store.getClientProxy()
- .getOMProxyProvider();
- List<OMProxyInfo> omProxies = omFailoverProxyProvider.getOMProxyInfos();
-
Review comment:
Can we add this test back, as you have exposed the
OMFailoverProxyProvider through
OMFailoverProxyProvider#getFailoverProxyProvider.
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OmTransportFactory.java
##########
@@ -0,0 +1,63 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.ozone.om.protocolPB;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.ServiceLoader;
+
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.security.UserGroupInformation;
+
+/**
+ * Factory pattern to create object for RPC communication with OM.
+ */
+public interface OmTransportFactory {
+
+ OmTransport createOmTransport(ConfigurationSource source,
+ UserGroupInformation ugi, String omServiceId) throws IOException;
+
+ static OmTransport create(ConfigurationSource conf,
+ UserGroupInformation ugi, String omServiceId) throws IOException {
+ OmTransportFactory factory = createFactory();
+
+ return factory.createOmTransport(conf, ugi, omServiceId);
+ }
+
+ static OmTransportFactory createFactory() throws IOException {
+ ServiceLoader<OmTransportFactory> transportFactoryServiceLoader =
+ ServiceLoader.load(OmTransportFactory.class);
+ Iterator<OmTransportFactory> iterator =
Review comment:
Question: We have not added any file with name
org.apache.hadoop.ozone.om.protocolPB.OmTransportFactory in meta-inf/services.
Then why we need this serviceLoader.
And just by creating the file named
org.apache.hadoop.ozone.om.protocolPB.OmTransportFactory with content
org.apache.hadoop.ozone.om.protocolPB.Hadoop3OmTransport. We don't need line
50:61 code right?
Not sure if I am missing something here.
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
##########
@@ -80,6 +65,21 @@
import org.apache.hadoop.security.UserGroupInformation;
import
org.apache.hadoop.security.authentication.client.AuthenticationException;
import org.apache.hadoop.test.GenericTestUtils;
+
+import org.apache.commons.io.FileUtils;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_HEARTBEAT_INTERVAL;
+import static
org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeState.HEALTHY;
+import static
org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_ADDRESS_KEY;
+import static
org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_DATANODE_ADDRESS_KEY;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.DFS_CONTAINER_IPC_PORT;
Review comment:
This has only import order change, can we avoid doing this, it will
cause conflicts to other PR's and also if any PR has changed it will cause
rebase problem to this also. Just a suggestion.
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java
##########
@@ -34,19 +39,13 @@
import org.apache.hadoop.ozone.recon.ReconServer;
import
org.apache.hadoop.security.authentication.client.AuthenticationException;
import org.apache.hadoop.test.GenericTestUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import java.io.IOException;
-import java.net.BindException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.concurrent.TimeoutException;
-import java.util.concurrent.TimeUnit;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
Review comment:
Same one here also, only import change
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/OmFailoverProxyUtil.java
##########
@@ -0,0 +1,50 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.ozone.om;
+
+import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
+import org.apache.hadoop.ozone.client.rpc.RpcClient;
+import org.apache.hadoop.ozone.om.ha.OMFailoverProxyProvider;
+import org.apache.hadoop.ozone.om.protocolPB.Hadoop3OmTransport;
+import
org.apache.hadoop.ozone.om.protocolPB.OzoneManagerProtocolClientSideTranslatorPB;
+
+/**
+ * Test utility to get the FailoverProxyProvider with cast.
+ */
+public final class OmFailoverProxyUtil {
+
+ private OmFailoverProxyUtil() {
+ }
+
+ /**
+ * Get FailoverProxyProvider from RpcClient.
+ */
+ public static OMFailoverProxyProvider getFailoverProxyProvider(
+ RpcClient clientProxy) {
+ OzoneManagerProtocolClientSideTranslatorPB ozoneManagerClient =
+ (OzoneManagerProtocolClientSideTranslatorPB) clientProxy
+ .getOzoneManagerClient();
+ Hadoop3OmTransport transport =
+ (Hadoop3OmTransport) ozoneManagerClient.getTransport();
+ return transport.getOmFailoverProxyProvider();
+ }
+
+ public static OMFailoverProxyProvider getFailoverProxyProvider(
Review comment:
One would have been sufficient right here, as anyway RpcClient extends
ClientProtocol.
So, we can have code as below right?
public static OMFailoverProxyProvider getFailoverProxyProvider(
ClientProtocol clientProtocol)) {
OzoneManagerProtocolClientSideTranslatorPB ozoneManagerClient =
(OzoneManagerProtocolClientSideTranslatorPB) clientProxy
.getOzoneManagerClient();
Hadoop3OmTransport transport =
(Hadoop3OmTransport) ozoneManagerClient.getTransport();
return transport.getOmFailoverProxyProvider();
}
##########
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -481,9 +484,6 @@ void cancelDelegationToken(Token<OzoneTokenIdentifier>
token)
*/
S3SecretValue getS3Secret(String kerberosID) throws IOException;
- @VisibleForTesting
Review comment:
I have a question, we are removing this public utility method from
client protocol, this will break applications if they use this method right,
after the upgrade. Or end users(clients) should not use the RpcClient class
directly only they should access through FileSystem and S3 Interfaces is the
contract?
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotProvider.java
##########
@@ -105,17 +106,15 @@ public void testDownloadCheckpoint() throws Exception {
retVolumeinfo.createBucket(bucketName);
OzoneBucket ozoneBucket = retVolumeinfo.getBucket(bucketName);
- String leaderOMNodeId = objectStore.getClientProxy().getOMProxyProvider()
- .getCurrentProxyOMNodeId();
- OzoneManager ozoneManager = cluster.getOzoneManager(leaderOMNodeId);
+ OzoneManager ozoneManager = cluster.getOMLeader();
Review comment:
The reason for this is
Because getOMLeader has chance to return null.
```
public OzoneManager getOMLeader() {
OzoneManager res = null;
for (OzoneManager ozoneManager : this.ozoneManagers) {
if (ozoneManager.isLeader()) {
if (res != null) {
// Found more than one leader
// Return null, expect the caller to retry in a while
return null;
}
// Found a leader
res = ozoneManager;
}
}
return res;
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
