[kudu-CR] [java client] Few ITClient improvements
Jean-Daniel Cryans has submitted this change and it was merged. Change subject: [java client] Few ITClient improvements .. [java client] Few ITClient improvements ITClient has been flaky for a while now, mostly due to the "Row count regressed" issue. I fixed it by using snapshot timestamps, which made me refactor how we build scanners, which made me add a new counting method in BaseKuduTest. I continued running the test and saw other issues. Some unchecked errors were not killing the test, so I added an UncaughtExceptionHandler. I also saw invalid scanner sequence ID errors that are normal due to how this test runs that were killing the test. Finally, I converted some plain Exceptions into KuduExceptions which gave us access to their Status. Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Reviewed-on: http://gerrit.cloudera.org:8080/4489 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo--- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java 2 files changed, 68 insertions(+), 26 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
Adar Dembo has posted comments on this change. Change subject: [java client] Few ITClient improvements .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java client] Few ITClient improvements
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4489 to look at the new patch set (#3). Change subject: [java client] Few ITClient improvements .. [java client] Few ITClient improvements ITClient has been flaky for a while now, mostly due to the "Row count regressed" issue. I fixed it by using snapshot timestamps, which made me refactor how we build scanners, which made me add a new counting method in BaseKuduTest. I continued running the test and saw other issues. Some unchecked errors were not killing the test, so I added an UncaughtExceptionHandler. I also saw invalid scanner sequence ID errors that are normal due to how this test runs that were killing the test. Finally, I converted some plain Exceptions into KuduExceptions which gave us access to their Status. Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java 2 files changed, 68 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4489/3 -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
Jean-Daniel Cryans has posted comments on this change. Change subject: [java client] Few ITClient improvements .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: PS1, Line 173: KuduScanner scanner = scanBuilder.build(); : while (scanner.hasMoreRows()) { : count += scanner.nextRows().getNumRows(); : } : return count; > Am I missing something? Why doesn't the following work: You're better at this than me. -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Few ITClient improvements
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4489 to look at the new patch set (#2). Change subject: [java client] Few ITClient improvements .. [java client] Few ITClient improvements ITClient has been flaky for a while now, mostly due to the "Row count regressed" issue. I fixed it by using snapshot timestamps, which made me refactor how we build scanners, which made me add a new counting method in BaseKuduTest. I continued running the test and saw other issues. Some unchecked errors were not killing the test, so I added an UncaughtExceptionHandler. I also saw invalid scanner sequence ID errors that are normal due to how this test runs that were killing the test. Finally, I converted some plain Exceptions into KuduExceptions which gave us access to their Status. Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java 2 files changed, 67 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4489/2 -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
Jean-Daniel Cryans has posted comments on this change. Change subject: [java client] Few ITClient improvements .. Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: Line 155: scanner.close(); > Hmm, didn't know we need to explicitly close. Guess I'm too used to the C++ You're right, here it's not needed. I copied the other count method (the async one) which was unnecessarily closing the scanner. You do need to close a scanner if you're not going to use it all the way through, else you leave junk in the TS. Closing it here is just a no-op on the client side since it knows the TS closed it when it ran out of data. PS1, Line 173: KuduScanner scanner = scanBuilder.build(); : while (scanner.hasMoreRows()) { : count += scanner.nextRows().getNumRows(); : } : return count; > Can you use countRowsInScan() here? Maybe elsewhere too? Not without some refactoring to make them accept predicates... Seems low gain. http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java: PS1, Line 107: thread.setUncaughtExceptionHandler(uncaughtExceptionHandler); > You find this cleaner than explicitly catching RuntimeException in writerTh Yes, yes. I was burned too often by dumb programming bug that'd show up in those threads that wouldn't kill the test. PS1, Line 290: if (resp == null) { : return false; : } > Why would the result of session.apply() be null, or session.flush() include If you manual flush, on apply you get null. There's a jira about this I think. Line 295: if (writeTimestamp != 0) { > Under what conditions will it be 0? The very first write? Doc was saying that it could be 0, over in the other patch David says it can't, so I'll remove this check. PS1, Line 419: so when we get back with the same one > Nit: this fragment is confusing. What does it mean to "get back with the sa I'll clarify. Basically, if you reconnect with the same TS, and your previous try was successful, then you get the error message. -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Few ITClient improvements
Adar Dembo has posted comments on this change. Change subject: [java client] Few ITClient improvements .. Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: Line 155: scanner.close(); Hmm, didn't know we need to explicitly close. Guess I'm too used to the C++ client. PS1, Line 173: KuduScanner scanner = scanBuilder.build(); : while (scanner.hasMoreRows()) { : count += scanner.nextRows().getNumRows(); : } : return count; Can you use countRowsInScan() here? Maybe elsewhere too? http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java: PS1, Line 107: thread.setUncaughtExceptionHandler(uncaughtExceptionHandler); You find this cleaner than explicitly catching RuntimeException in writerThread and scannerThread (is this an issue for chaosThread too)? PS1, Line 290: if (resp == null) { : return false; : } Why would the result of session.apply() be null, or session.flush() include a null in the list? Line 295: if (writeTimestamp != 0) { Under what conditions will it be 0? The very first write? PS1, Line 419: so when we get back with the same one Nit: this fragment is confusing. What does it mean to "get back with the same one"? -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Few ITClient improvements
Jean-Daniel Cryans has uploaded a new change for review. http://gerrit.cloudera.org:8080/4489 Change subject: [java client] Few ITClient improvements .. [java client] Few ITClient improvements ITClient has been flaky for a while now, mostly due to the "Row count regressed" issue. I fixed it by using snapshot timestamps, which made me refactor how we build scanners, which made me add a new counting method in BaseKuduTest. I continued running the test and saw other issues. Some unchecked errors were not killing the test, so I added an UncaughtExceptionHandler. I also saw invalid scanner sequence ID errors that are normal due to how this test runs that were killing the test. Finally, I converted some plain Exceptions into KuduExceptions which gave us access to their Status. Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 --- M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java 2 files changed, 70 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4489/1 -- To view, visit http://gerrit.cloudera.org:8080/4489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans