You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/01/31 01:19:57 UTC
[kudu-CR] [java] better client and minicluster cleanup after tests finish
Hello Alexey Serbin, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12322
to review the following change.
Change subject: [java] better client and minicluster cleanup after tests finish
......................................................................
[java] better client and minicluster cleanup after tests finish
Many tests create their own client and minicluster instances, but don't take
care to close them properly in all cases. This patch fixes that by:
1. Converting all temporary instantations to use try-with-resources.
2. For non-temporary instantiations, ensuring that close is always called,
even if an exception is thrown mid-cleanup.
The most interesting change is probably to KuduContext.scala, where we now
close clients when clearing out the KuduClientCache (called by an @After).
To avoid double closing, we need to unregister a client's shutdown hook
after closing it.
Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSink.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java
10 files changed, 196 insertions(+), 200 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12322/1
--
To view, visit http://gerrit.cloudera.org:8080/12322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
Gerrit-Change-Number: 12322
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] [java] better client and minicluster cleanup after tests finish
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12322 )
Change subject: [java] better client and minicluster cleanup after tests finish
......................................................................
[java] better client and minicluster cleanup after tests finish
Many tests create their own client and minicluster instances, but don't take
care to close them properly in all cases. This patch fixes that by:
1. Converting all temporary instantations to use try-with-resources.
2. For non-temporary instantiations, ensuring that close is always called,
even if an exception is thrown mid-cleanup.
The most interesting change is probably to KuduContext.scala, where we now
close clients when clearing out the KuduClientCache (called by an @After).
To avoid double closing, we need to unregister a client's shutdown hook
after closing it.
Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
Reviewed-on: http://gerrit.cloudera.org:8080/12322
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSink.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java
10 files changed, 196 insertions(+), 200 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, but someone else must approve
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
Gerrit-Change-Number: 12322
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] better client and minicluster cleanup after tests finish
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12322 )
Change subject: [java] better client and minicluster cleanup after tests finish
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12322/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12322/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1060
PS1, Line 1060: AsyncKuduClient asyncKuduClient = new AsyncKuduClient
: .AsyncKuduClientBuilder(harness.getMasterAddressesAsString())
: .defaultAdminOperationTimeoutMs(harness.DEFAULT_SLEEP)
: .build();
BTW, why not to put this AsyncKuduClient instance into the auto-close closure? I would expect that way the access to the async- and the derived sync-client would be automatically restricted to a closure where they are valid. Why putting just the derived KuduClient is better?
--
To view, visit http://gerrit.cloudera.org:8080/12322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
Gerrit-Change-Number: 12322
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Jan 2019 20:03:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] better client and minicluster cleanup after tests finish
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12322 )
Change subject: [java] better client and minicluster cleanup after tests finish
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
Gerrit-Change-Number: 12322
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Jan 2019 20:53:12 +0000
Gerrit-HasComments: No
[kudu-CR] [java] better client and minicluster cleanup after tests finish
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12322 )
Change subject: [java] better client and minicluster cleanup after tests finish
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12322/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12322/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1060
PS1, Line 1060: AsyncKuduClient asyncKuduClient = new AsyncKuduClient
: .AsyncKuduClientBuilder(harness.getMasterAddressesAsString())
: .defaultAdminOperationTimeoutMs(harness.DEFAULT_SLEEP)
: .build();
> BTW, why not to put this AsyncKuduClient instance into the auto-close closu
The sync client isn't a separate client instance; it's actually just a sync wrapper around the async client. As such, closing the sync client closes the underlying async client. So there's no reason to close both; in fact, the second close will throw an exception.
--
To view, visit http://gerrit.cloudera.org:8080/12322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If91b3b2787915f0361537dd999e0daa8f7cb0a36
Gerrit-Change-Number: 12322
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Jan 2019 20:36:46 +0000
Gerrit-HasComments: Yes