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 <jdcry...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to