You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2021/05/23 16:27:51 UTC
[kudu-CR] [java] KUDU-3287: Improve the Java client close behavior
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17488
Change subject: [java] KUDU-3287: Improve the Java client close behavior
......................................................................
[java] KUDU-3287: Improve the Java client close behavior
This patch improves the Java client close behavior to prevent resources
that live for some time after close() has returned. It does this by
blocking ReleaseResourcesCB on shutdownGracefully. It also ensures
the internal ExecutorService is shutdown.
Change-Id: I1efdb459881bb8e52db18dc33abcece63b8e8030
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
4 files changed, 81 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/17488/1
--
To view, visit http://gerrit.cloudera.org:8080/17488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1efdb459881bb8e52db18dc33abcece63b8e8030
Gerrit-Change-Number: 17488
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [java] KUDU-3287: Improve the Java client close behavior
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17488 )
Change subject: [java] KUDU-3287: Improve the Java client close behavior
......................................................................
Patch Set 2:
(6 comments)
Nice find!
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@309
PS2, Line 309: KuduSession
If this is a test scenario centers around KuduSession, not AsyncKuduSession, why not to put this new scenarios into TestKuduSession.java?
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@312
PS2, Line 312: session.apply(createBasicSchemaInsert(table, i));
Does it make sense to check for the status of KuduSessionApply()? Since the session in AUTO_FLUSH_SYNC mode, apply() returns non-null objects
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@314
PS2, Line 314: session.flush();
Is this crucial? IIRC, the session is by default in AUTO_FLUSH_SYNC mode, meaning every row is flushed upon call to KuduSession.apply().
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@334
PS2, Line 334: int afterAliveThreads = 0;
: int afterKuduNioThreads = 0;
: for (Thread thread : Thread.getAllStackTraces().keySet()) {
: if (thread.isAlive()) {
: afterAliveThreads++;
: if (thread.getName().contains("kudu-nio-")) {
: afterKuduNioThreads++;
: }
: }
: }
How fast does joining the threads happen? Is it dependent on scheduler anomalies of any sorts, or client.close() enforces that all corresponding threads have joined? Would be great to add a comment on that.
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@347
PS2, Line 347: assertEquals(0, afterKuduNioThreads);
Is there any absolute-style post-condition to enforce on afterAliveThreads (like it's for afterKuduNioThreads)?
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java:
http://gerrit.cloudera.org:8080/#/c/17488/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java@164
PS2, Line 164: !client.isClosed()
I'm curious: what happens if calling shutdown() on already closed client? From the code it looks like a no-op, and I'd expect it to be safe, no?
--
To view, visit http://gerrit.cloudera.org:8080/17488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1efdb459881bb8e52db18dc33abcece63b8e8030
Gerrit-Change-Number: 17488
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 23 May 2021 22:13:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-3287: Improve the Java client close behavior
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17488
to look at the new patch set (#2).
Change subject: [java] KUDU-3287: Improve the Java client close behavior
......................................................................
[java] KUDU-3287: Improve the Java client close behavior
This patch improves the Java client close behavior to prevent resources
that live for some time after close() has returned. It does this by
blocking ReleaseResourcesCB on shutdownGracefully. It also ensures
the internal ExecutorService is shutdown.
Change-Id: I1efdb459881bb8e52db18dc33abcece63b8e8030
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
4 files changed, 79 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/17488/2
--
To view, visit http://gerrit.cloudera.org:8080/17488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1efdb459881bb8e52db18dc33abcece63b8e8030
Gerrit-Change-Number: 17488
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)