David Ribeiro Alves 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: (13 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/ 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 methods/fields as strictly consistent between clients as possible 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 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 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 client.updateLastPropagatedTimestamp() only once. more generally though I'm not sure I understand what this is doing, likely worth it to add a comment explaining when and why we get these timestamps and why we chose the one we chose. http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@817 PS7, Line 817: // If the last propagated timestamp is set send it with the scan. nit, comma after "set" http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@818 PS7, Line 818: propagation timestamp pet peeve: propagated timestamp or propagation timestamp? are these different? if not maybe choose one and use it consistently, likely too late for the other client, but still on time for this one. 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@1044 PS7, Line 1044: running nit: "are running" or just "run" http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1047 PS7, Line 1047: // scan mode from leader replica. In this test writes are nit: comma after "scan mode" http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1062 PS7, Line 1062: // This is a test that verifies, when multiple clients running : // simultaneously, a client can get read-your-writes and : // read-your-reads session guarantees using READ_YOUR_WRITES : // scan mode from leader replica This text is repeated. Likely add it to the first test and then make the other tests point to it. 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 use executors/tasks and futures? http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1086 PS7, Line 1086: 4 avoid magic numbers (set a final var) 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... -- 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 <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 05 Mar 2018 23:14:09 +0000 Gerrit-HasComments: Yes
