[GitHub] [ozone] elek commented on a change in pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient

[GitBox <gi...@apache.org>]

elek commented on a change in pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#discussion_r631961732



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##########
@@ -248,6 +249,22 @@ public void onRemoval(
         }).build();
   }
 
+  @NotNull
+  @VisibleForTesting
+  protected XceiverClientFactory createXceiverClientFactory(
+      ConfigurationSource configSource,
+      List<X509Certificate> x509Certificates) throws IOException {
+    return new XceiverClientManager(configSource,
+        conf.getObject(XceiverClientManager.ScmClientConfig.class),

Review comment:
       Good question (and thanks the review)
   
   `conf` is a private field, so without an additional getter (supposed to be `@VisibleForTesting`) it's not available in the subclasses. That may be my original motivation ;-)
   
   But I just realized that `configSource` is not required in the subclasses when I override the `createXceiverClientFactory`, so I removed it all together:
   
   {code}
        @NotNull
         @Override
         protected XceiverClientFactory createXceiverClientFactory(
             List<X509Certificate> x509Certificates)
             throws IOException {
           return new MockXceiverClientFactory();
         }
   {code}
   
   And started to use `conf` field in the main implementation as you suggested.




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org