You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/04/09 13:02:49 UTC
[GitHub] [ozone] elek opened a new pull request #2138: Create unit (!) test for OzoneClient
elek opened a new pull request #2138:
URL: https://github.com/apache/ozone/pull/2138
## What changes were proposed in this pull request?
Today `OzoneClient` is tested only by integration tests (`TestOzoneRpcClientAbstract` from integration-test and all the sub-classes).
I believe `OzoneClient` itself contains a lot of smart logic (especially regarding the buffering and retry/commit) which are good to be tested. This test can be faster and more efficient with real unit tests where the network traffic is mocked.
Fortunately we already have interfaces for both (`OmTransport` and `XceiverClientSpi`. The latter is just an abstract class, not an interface, but nothing can be perfect ;-)).
With the help of these interfaces it's quite easy to create the unit test.
I copied some methods from the `TestOzoneRpcClientAbstract` and tested them without creating MiniOzoneCluster but using in-memory implementation of OM/DN client interfaces.
Not all the methods are tested (yet) but the main key create / read path is already covered. The main goal is to have all the required classes on master which can be used later by the new EC part of the client which can have good coverage with standard, fast unit tests (and later with integration test, too...)
## How was this patch tested?
The new unit test is executed.
--
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
[GitHub] [ozone] elek merged pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
elek merged pull request #2138:
URL: https://github.com/apache/ozone/pull/2138
--
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
[GitHub] [ozone] elek commented on pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#issuecomment-842110665
Thanks for the review @hanishakoneru. Merging it now...
--
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
[GitHub] [ozone] elek commented on pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#issuecomment-836282813
Can you please help me with a review @umamaheswararao?
--
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
[GitHub] [ozone] elek commented on a change in pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by 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
[GitHub] [ozone] hanishakoneru commented on a change in pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#discussion_r630526952
##########
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:
Here, configSource = conf. Is there any use case for setting the ConfigurationSource separately from ScmClientConfig?
--
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
[GitHub] [ozone] bshashikant commented on pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#issuecomment-826642780
@elek , the changes look good. Can you please rebase?
--
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
[GitHub] [ozone] elek commented on pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#issuecomment-826668512
> the changes look good so far and i am still reviewing the changes. Meanwhile, Can you please rebase?
Sure, it's rebased now. (And thank you for checking it).
--
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
[GitHub] [ozone] bshashikant commented on pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#issuecomment-826643357
> @elek , the changes look good so far and i am still reviewing the changes. Meanwhile, Can you please rebase?
--
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
[GitHub] [ozone] elek commented on pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2138:
URL: https://github.com/apache/ozone/pull/2138#issuecomment-842110665
Thanks for the review @hanishakoneru. Merging it now...
--
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
[GitHub] [ozone] elek merged pull request #2138: HDDS-5082. Create unit (!) test for OzoneClient
Posted by GitBox <gi...@apache.org>.
elek merged pull request #2138:
URL: https://github.com/apache/ozone/pull/2138
--
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