Adar Dembo has posted comments on this change.

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

Patch Set 4:

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

Line 810:       } else {
This will need a rebase + conflict resolution.

Line 1152:       Map.Entry<byte[], byte[]> nonCoveredRange = 
getNonCoveredRange(table.getTableId(), key);
Can use tableId here.
File java/kudu-client/src/main/java/org/kududb/client/

Line 327:           operation.callback(response);
Does this invoke the user callback eventually? If so, why do it before adding 
the error to the error collector?

PS4, Line 784: private Object tablet = null;
Ugh. Can you add more color on why this approach was necessary? Why can't we 
store the results in two separate fields, one LocatedTablet and one exception?
File java/kudu-client/src/main/java/org/kududb/client/

Line 84:   @Override
> if concurrent updates happen to the entries the iteration is undefined, so 
Really? I thought ConcurrentSkipListMap defines an iteration order in the face 
of concurrent mutations. Javadoc says: "Insertion, removal, update, and access 
operations safely execute concurrently by multiple threads. Iterators are 
weakly consistent, returning elements reflecting the state of the map at some 
point at or since the creation of the iterator. They do not throw 
ConcurrentModificationException, and may proceed concurrently with other 

Line 89:     for (Map.Entry<byte[], byte[]> range : 
nonCoveredRanges.entrySet()) {
> If you used a Guava Joiner to convert this list of Strings into a single co
No love for Joiner? Bear in mind size() is more expensive on a 
File java/kudu-client/src/test/java/org/kududb/client/

Line 221:   public void testInsertManualFlushNonCoveredRange() throws Exception 
> Replicas aren't important for these particular tests.  Additionally, they c
Right, but I don't think the other tests in this class particularly care about 
replication either (testUpsert() for example). Could you make a pass over all 
of the TestKuduSession tests and switch every test to single-replica, if it 
makes sense? It's more cognitive load as it is now; the reader's eyes are drawn 
to the inconsistency.

To view, visit
To unsubscribe, visit

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

Reply via email to