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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 23 May 2021 22:13:30 +0000 Gerrit-HasComments: Yes
