Dan Burkert 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 thro
Done


Line 684:           // TODO: should this throw instead?
> Unit test it and see what happens? :)
Woops, these were leftover from before NonCoveredRangeException.  Ideally 
NonCoveredRangeException wouldn't exist and it would just be a normal 
KuduException with a status that indicates a non covered range, but until your 
error handling cleanup, this will have to do.


Line 1371:     if (locations.isEmpty()) {
> How does this happen? Remove all the tablets?
Yah, currently this never happens since you can't create an empty table, and 
you can't remove ranges.  Eventually it will be possible.  I've expanded the 
comment a bit, since the logic about why it indicates an empty table is pretty 
subtle.


PS1, Line 1376: then
> the*
Done


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? :)
Done


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?
Done


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.
Done


Line 86:   boolean partitionKeyOrNext() {
> I was expecting this to be used for scans, but it's not overrode anywhere?
It was overriden in the async kudu scanner, but I ended up simplifying this out 
by just using NonCoveredRangeException everywhere (and the scanner specifically 
recovers from it).


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?
woops, holdover from debugging.


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.
Done


PS1, Line 23: AsyncKuduClient
> Using that on purpose?
Done


Line 26:   private final ConcurrentNavigableMap<byte[], byte[]> 
nonCoveredRanges =
> Another case where we don't seem to invalidate the cache. In fact, we don't
Added a note to the javadoc.  This will come later, but this class gives us a 
good location to put the logic.


Line 30:    *
> Finish the javadoc.
Done


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


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.
Done


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


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 pos
Done


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
Done


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.
Done


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.
Done


Line 467:     assertEquals(0, countRowsForTestScanNonCoveredTable(table, null, 
-1));
> I'm not sure looking at this if we're expecting 0 rows because none are ins
This tests inserts a value into every row in the covered range. This particular 
instance is only scanning non-covered partition space, so there are no results.


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.
Done


Line 290: 
> No negative testing?
These tests are only testing what happens when inserting into non-covered 
ranges.  The tests which scan non-covered ranges all insert into covered ranges 
within tables with non-covered ranges.


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

Reply via email to