Dan Burkert has posted comments on this change. Change subject: [java-client] Add ScanToken.stringifySerializedToken ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/4173/1//COMMIT_MSG 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 c=) > 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 parsing? http://gerrit.cloudera.org:8080/#/c/4173/1/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java 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 Done 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 https://github.com/apache/kudu/blob/master/src/kudu/common/partition-test.cc#L474. 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 range. http://gerrit.cloudera.org:8080/#/c/4173/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java 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. http://gerrit.cloudera.org:8080/#/c/4173/1/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java: Line 19: import org.apache.kudu.ColumnSchema; > Where is this used? Done http://gerrit.cloudera.org:8080/#/c/4173/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: Line 413: LOG.debug(KuduScanToken.stringifySerializedToken(token.serialize(), syncClient)); > Not debug() here? Done -- 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
