Dan Burkert has posted comments on this change.

Change subject: [java-client] Add ScanToken.stringifySerializedToken

Patch Set 2:


Commit Message:

PS1, Line 16: ScanToken
> I'm having a hard time understanding this example- what would the partition
The partition that the token covers is determined by the hash buckets and range 
partition bounds.

PS1, Line 17: lower-bound-primary-key=(string a=1, string b=3, string c=5),
            :           upper-bound-primary-key=(string a=2, string b=4, string 
> If I'm not mistaken, these are implied by the partitioning, right? I guess 
The primary key bounds aren't very useful to Impala currently, but they will be 
once token splitting is implemented and used.

PS1, Line 25: I haven't added tests of the specific
            : format of the strings
> The motivation for this was that Impala has a suite of planner tests that v
I'm not crazy about committing to a specific format, but I think it's fine to 
say that this will be a string that is useful for debugging scan token issues.  
I was under the impression that this was not something that Impala will be 

File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

Line 313:     // bytes in the input are escaped as 0x0001.
> Would be nice to see a comment explaining why the below code is the way it 

PS1, Line 364:     // Even though we parse hash buckets for the upper and lower 
bound partition
             :     // keys, we only use the lower bound set. Upper bound 
partition keys are
> Maybe some sort of assertion that lower and upper agree on hash buckets?
The upper bound partition key of a tablet is exclusive, and therefore may 
contain an incremented hash bucket, for a concrete example see 

I agree throwing away the upper bound hash buckets is kind of weird, but the 
alternative of just parsing out the range portion is not trivial since the key 
must be checked for sufficient length.  I think it's easier to just leave it 
and reuse that logic in decodePartitionKey.

PS1, Line 401: (
> ]
We use this notation to indicate that its a lower-inclusive/upper-exclusive 

File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java:

Line 102:   public static KuduScanner deserializeIntoScanner(byte[] buf, 
KuduClient client) throws IOException {
> Does this need to be in the release notes?
eh maybe.

File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java:

Line 19: import org.apache.kudu.ColumnSchema;
> Where is this used?

File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 413:       
> Not debug() here?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I42014da270e66c370cc6d6ed68fb38331130ff6d
Gerrit-PatchSet: 2
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: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to