[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8847 ) Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 Reviewed-on: http://gerrit.cloudera.org:8080/8847 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 4 files changed, 236 insertions(+), 7 deletions(-) Approvals: Kudu Jenkins: Verified David Ribeiro Alves: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 10: as discussed offline, ok with merging this as long as we make sure that we're going to finish the jepsen part of the tests -- 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: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 12 Mar 2018 21:07:00 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 10: Code-Review+2 -- 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: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 12 Mar 2018 21:06:25 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 10: lgtm, thanks waiting to get some validation from the jepsen work and then it's gtg -- 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: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 08 Mar 2018 23:46:54 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 10: Code-Review+1 -- 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: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 08 Mar 2018 23:45:49 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/8847/9/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/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1085 PS9, Line 1085: tasksNum > tiny nit: java uses camel case :) Right... thanks for catching it. http://gerrit.cloudera.org:8080/#/c/8847/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1144 PS9, Line 1144: Waits for th > add a note indicating that any exceptions or assertion error will bubble up Done -- 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: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 08 Mar 2018 21:50:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#10). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 4 files changed, 236 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/10 -- 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: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/8847/9/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/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1085 PS9, Line 1085: tasks_num tiny nit: java uses camel case :) http://gerrit.cloudera.org:8080/#/c/8847/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1144 PS9, Line 1144: future.get(); add a note indicating that any exceptions or assertion error will bubble up here. -- 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: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 08 Mar 2018 21:18:04 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 9: (1 comment) 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@1150 PS7, Line 1150: : : : > right this kind of mess is why futures are much better (and the impl in lin I see, makes sense. Updated. -- 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: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 08 Mar 2018 00:48:51 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#9). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 4 files changed, 233 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/9 -- 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: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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:
(1 comment)
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@1150
PS7, Line 1150: // Fail the main thread if ever encounter AssertionError.
: if (assertionError != null) {
: fail("the test should not throw AssertionError: " +
assertionError);
: }
> Since the JUnit only captures assertion errors in the main thread of the te
right this kind of mess is why futures are much better (and the impl in line
918 seems to have a bug btw).
you could not catch the exception in the thread and just have
ExecutorService threadPool = Executors.newFixedThreadPool(3);
Future future = threadPool.submit(new MyTest());
when you do future.get() if an exception occurred it'll propagate to the caller
thread, no need to catch any exception inside runnable
--
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
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 06 Mar 2018 22:43:55 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 8: Verified+1 The failed test is unrelated flaky -- 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: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 06 Mar 2018 01:26:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hao Hao has removed a vote on this change. Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. Removed Verified-1 by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG@6 PS7, Line 6: : KUDU-1704: add java client support for READ_YOUR_WRITES mode > ok, thats fine. really want to make sure we don't drop it though, if we're Yeah, totally makes sense to me. I will certainly pick up the jepsen test with RYW. 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@817 PS7, Line 817: case OPENING: > nit, comma after "set" Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@818 PS7, Line 818: nner. This kind of a > pet peeve: propagated timestamp or propagation timestamp? are these differe Done 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: run > nit: "are running" or just "run" Done 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" Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1062 PS7, Line 1062: // Similar to testReadYourWritesSyncLeaderReplica, but in this : // test writes are performed in MANUAL_FLUSH (batches) flush modes. : @Test(timeout = 10) : public void testReadYourWritesBa > This text is repeated. Likely add it to the first test and then make the ot Done http://gerrit.cloudera.org:8080/#/c/8847/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1086 PS7, Line 1086: > avoid magic numbers (set a final var) Done -- 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: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 06 Mar 2018 00:35:23 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#8). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 4 files changed, 241 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/8 -- 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: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG@6
PS7, Line 6:
: KUDU-1704: add java client support for READ_YOUR_WRITES mode
> Unfortunately, did not get a change to spend much time on it yet, but shoul
ok, thats fine. really want to make sure we don't drop it though, if we're
willing to go the distance in implementing RYW we should also test it properly.
super weird corner cases often pop up and jepsen is one of the best tools to
let you know that went wrong and why.
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@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);
: }
> I guess you are saying the comment above is not clear enough. Will update.
yeah, that's why I said slightly simplified. I agree that we might want to keep
the flow for consistency.
the point about the comment was more important though
--
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
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 06 Mar 2018 00:13:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG@6 PS7, Line 6: : KUDU-1704: add java client support for READ_YOUR_WRITES mode > more generally, how's the jepsen test going? it'd be a great validation of Unfortunately, did not get a change to spend much time on it yet, but should wrap up other things I am working on soon. -- 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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 06 Mar 2018 00:01:48 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:59:21 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8847/7//COMMIT_MSG@6 PS7, Line 6: : KUDU-1704: add java client support for READ_YOUR_WRITES mode more generally, how's the jepsen test going? it'd be a great validation of this client's implementation, since it uses it. -- 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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 05 Mar 2018 23:15:27 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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: I6239521c022147257859e399f55c6f3f
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#6). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 4 files changed, 229 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/6 -- 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: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#5). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 4 files changed, 196 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/5 -- 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: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
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 4: I'm slacking on this review a bit until we have something solid on the c++ side (which we seem close to now). it's easier to review by analogy :) -- 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: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 21 Feb 2018 00:44:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#4). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/TestScannerMultiTablet.java 5 files changed, 230 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/4 -- 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: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
