Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8847 )
Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@133 PS7, Line 133: return > nit: s/thus return/thus may return/ Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@200 PS7, Line 200: propagationTimestamp > in the c++ client this has a different name, right? please keep names for m Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@315 PS7, Line 315: unnecessarily wait. > grammar Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@415 PS7, Line 415: updates > s/updates/update Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@418 PS7, Line 418: if (readMode == ReadMode.READ_YOUR_WRITES && : resp.scanTimestamp != AsyncKuduClient.NO_TIMESTAMP) { : client.updateLastPropagatedTimestamp(resp.scanTimestamp); : } else if (resp.propagatedTimestamp != AsyncKuduClient.NO_TIMESTAMP) { : client.updateLastPropagatedTimestamp(resp.propagatedTimestamp); : } > this could be slightly simplified (choose a timestamp with the ifs, call cl I guess you are saying the comment above is not clear enough. Will update. So in general, for READ_YOUR_WRITES since we return the chosen snapshot timestamp, we should use it for as the propagation timestamp to avoid bump up the propagation timestamp unnecessarily. Since for RYW scan it is good, as long as the chosen timestamp of next read is greater than the previous one. There is an identical flow in C++ Client https://gerrit.cloudera.org/#/c/8823/20/src/kudu/client/scanner-internal.cc@491 http://gerrit.cloudera.org:8080/#/c/8847/7/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/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1080 PS7, Line 1080: readYourWrites > do other java tests that have concurrency also use raw threads? or do they There is another java test inside this test class use raw threads. https://gerrit.cloudera.org/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@918 http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1150 PS7, Line 1150: // Fail the main thread if ever encounter AssertionError. : if (assertionError != null) { : fail("the test should not throw AssertionError: " + assertionError); : } > hum, not sure what this is doing... Since the JUnit only captures assertion errors in the main thread of the test, and it is not aware of exceptions from within new spawn threads. I added this line to fail the main thread if there is any error in the spawn ones. Will update the comment to be more clear. -- To view, visit http://gerrit.cloudera.org:8080/8847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 05 Mar 2018 23:59:21 +0000 Gerrit-HasComments: Yes
