HDDS-3500. Hide OMFailoverProxyProvider usage behind an interface #900
Conversation
|
@bharatviswa504 Can you please help me the review? |
There was a problem hiding this comment.
Thanks @elek for working on this. The change LGTM. Tested by (currently disabled) OM HA acceptance test (after fixing the Dockerfile for HDDS-3618):
$ cd hadoop-ozone/dist/target/ozone-0.6.0-SNAPSHOT/compose/ozone-om-ha
$ ./test_disabled.sh
...
ozone-om-ha-testOMHA :: Smoketest ozone cluster startup
==============================================================================
Stop Leader OM and Verify Failover | PASS |
------------------------------------------------------------------------------
Test Multiple Failovers | PASS |
------------------------------------------------------------------------------
Restart OM and Verify Ratis Logs | PASS |
------------------------------------------------------------------------------
ozone-om-ha-testOMHA :: Smoketest ozone cluster startup | PASS |
3 critical tests, 3 passed, 0 failed
3 tests total, 3 passed, 0 failed
@elek I will take a look at it today. |
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/Hadoop3OmTransport.java
Outdated
Show resolved
Hide resolved
...ne/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/Hadoop3OmTransportFactory.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OmTransport.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank You @elek for the contribution and now code looks cleaner with separation.
Overall LGTM, I have a few minor comments.
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java
Outdated
Show resolved
Hide resolved
| static OmTransportFactory createFactory() throws IOException { | ||
| ServiceLoader<OmTransportFactory> transportFactoryServiceLoader = | ||
| ServiceLoader.load(OmTransportFactory.class); | ||
| Iterator<OmTransportFactory> iterator = |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good question. This is because we need a default and an option to choose a different implementation.
If we create a service file for the default transport factory (Hadoop3OmTransportFactory) than it would be hard to override it. In case of the Hadoop2 client we would have two different implementation defined without an easy way to choose between them.
With this approach the default is always defined by line 50:61 but you have the option to define your own version.
There was a problem hiding this comment.
Question: what is alternative way other then having meta-inf/services to load class for OmTransportFactory?
There was a problem hiding this comment.
Currently there is no alternative way. I couldn't see any usage for any other approach, but if you see any use case, I am open to implement it. Let's create a new issue if you see some interesting direction...
There was a problem hiding this comment.
Then I am confused, what is the purpose of the code?
Because I think meta-inf/services is packaged along with jar right?
...ration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
Show resolved
Hide resolved
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/OmFailoverProxyUtil.java
Outdated
Show resolved
Hide resolved
...test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotProvider.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
Show resolved
Hide resolved
|
Thanks the review and the suggestions @bharatviswa504, I addressed all the comments and pushed the improved version. |
|
I have one question, other than that it LGTM. |
|
Thanks the review @bharatviswa504 and @adoroszlai I am pushing it now. @bharatviswa504 If you think we need to improve the pluggability, let's continue this work and create new issues... |
|
I have a question, and posted my comments to the discussion. |
|
@bharatviswa504 Please check #992 about the usage of META-INF/service. We can further improve in there, if you have any specific suggestion. |
What changes were proposed in this pull request?
OzoneManagerProtocolClientSideTranslatorPBusesOmFailoverProxyProviderto access OM HA, but this class is not supported in Hadoop 2.x environment.It would be better to
ProtocolClientSideTranslatorfrom the transport layer logicOzoneManagerProtocol(getOMFailoverProxyProvidershould be removed)OMTransportinterface to handle all the connection logic in one, isolated placeWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3500
How was this patch tested?
With my own form