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:

File java/kudu-client/src/main/java/org/kududb/client/

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
File java/kudu-client/src/main/java/org/kududb/client/

Line 443:           //" is returning rows: " +;
woops, we've been carrying this for a while. Please remove? :)
File java/kudu-client/src/main/java/org/kududb/client/

I thought it was only for manual flush?
File java/kudu-client/src/main/java/org/kududb/client/

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?
File java/kudu-client/src/main/java/org/kududb/client/

Line 57:     RowResultIterator i = d.join(asyncScanner.scanRequestTimeout);
Why was this this necessary?
File java/kudu-client/src/main/java/org/kududb/client/

Line 1: package org.kududb.client;

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.

Line 1: package org.kududb.client;

Line 6: public class NonCoveredRangeException extends KuduException {
Missing interface audience.
File java/kudu-client/src/main/java/org/kududb/client/

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?
File java/kudu-client/src/test/java/org/kududb/client/

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

Line 249:   @Test
Add timeouts here and below.
File java/kudu-client/src/test/java/org/kududb/client/

PS1, Line 412: .
remove period.

Line 467:     assertEquals(0, countRowsForTestScanNonCoveredTable(table, null, 
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.
File java/kudu-client/src/test/java/org/kududb/client/

Line 221:     String tableName = TABLE_NAME_PREFIX + 
Seems like those first lines can be refactored out in all tests.

Line 290: 
No negative testing?

To view, visit
To unsubscribe, visit

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

Reply via email to