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: (7 comments) http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: 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. http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java: 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? http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java: 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 operations." 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 ConcurrentSkipListMap. http://gerrit.cloudera.org:8080/#/c/3388/2/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: 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 http://gerrit.cloudera.org:8080/3388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> 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
