You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org> on 2016/09/21 01:27:02 UTC
[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 <jd...@apache.org>
[kudu-CR] [java client] Few ITClient improvements
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Few ITClient improvements
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
---
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 <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
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 Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
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 Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Few ITClient improvements
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
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 Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Few ITClient improvements
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Few ITClient improvements
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
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 Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Few ITClient improvements
......................................................................
Patch Set 2:
(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: }
:
: protected List<String> scanTableToStrings(KuduTable table,
: KuduPredicate... predicates) throws Exception {
: List<String>
> Not without some refactoring to make them accept predicates... Seems low ga
Am I missing something? Why doesn't the following work:
KuduScanner.KuduScannerBuilder scanBuilder = syncClient.newScannerBuilder(table);
for (KuduPredicate predicate : predicates) {
scanBuilder.addPredicate(predicate);
}
scanBuilder.setProjectedColumnIndexes(ImmutableList.<Integer>of());
return countRowsInScan(scanBuilder.build());
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes