Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1309: [java client] support tables with non-covering 
partition-key ranges
......................................................................


Patch Set 1:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 154:    * Map of table ID to non-covered range cache.
Can you describe the lifecycle of items the get into this cache? Going through 
the code it doesn't seem like we invalidate it.


Line 684:           // TODO: should this throw instead?
Unit test it and see what happens? :)


Line 1371:     if (locations.isEmpty()) {
How does this happen? Remove all the tablets?


PS1, Line 1376: then
the*


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java:

Line 443:           //LOG.info("Scan.open is returning rows: " + 
resp.data.getNumRows());
woops, we've been carrying this for a while. Please remove? :)


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

PS1, Line 121: AUTO_FLUSH_BACKGROUND
I thought it was only for manual flush?


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:

PS1, Line 84: .
nit: don't end with period here.


Line 86:   boolean partitionKeyOrNext() {
I was expecting this to be used for scans, but it's not overrode anywhere?


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java:

Line 57:     RowResultIterator i = d.join(asyncScanner.scanRequestTimeout);
Why was this this necessary?


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:

Line 1: package org.kududb.client;
License.


PS1, Line 23: AsyncKuduClient
Using that on purpose?


Line 26:   private final ConcurrentNavigableMap<byte[], byte[]> 
nonCoveredRanges =
Another case where we don't seem to invalidate the cache. In fact, we don't 
seem to be able to override it either.


Line 30:    *
Finish the javadoc.


Line 45:   public void addNonCoveredRange(byte[] startPartitionKey, byte[] 
endPartitionKey) {
Add javadoc.


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
File 
java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java:

Line 1: package org.kududb.client;
License.


Line 6: public class NonCoveredRangeException extends KuduException {
Missing interface audience.


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java:

Line 79:     return new RowResultIterator(0, null, null, null, null);
Should we consider keeping a single static instance? I don't think it's 
possible to modify them once they return null, so they're safe to reuse?


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java:

Line 239:   public static CreateTableOptions 
getBasicTableOptionsWithNonCoveredRange() {
Put in the javadoc a quick representation of what the table ends up looking 
like.


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
File 
java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java:

Line 249:   @Test
Add timeouts here and below.


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java:

PS1, Line 412: .
remove period.


Line 467:     assertEquals(0, countRowsForTestScanNonCoveredTable(table, null, 
-1));
I'm not sure looking at this if we're expecting 0 rows because none are 
inserted in that range or if we're in non-covered space. Also, not sure if 
you're testing a scan that starts in non-covered territory.


http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:

Line 221:     String tableName = TABLE_NAME_PREFIX + 
"-manual-flush-non-covered-range";
Seems like those first lines can be refactored out in all tests.


Line 290: 
No negative testing?


-- 
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to