Adar Dembo has posted comments on this change. Change subject: [java-client] Add ScanToken.stringifySerializedToken ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4173/1//COMMIT_MSG Commit Message: PS1, Line 25: I haven't added tests of the specific : format of the strings > If we're considering this to be a stable-ish interface, or at least enough I think this is just for debug printing, at least according to what Dan writes above. I'd be nervous about making it stable. What would be the use case? 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: private static byte[] decodeNonTerminalBinaryColumn(ByteBuffer key) { Would be nice to see a comment explaining why the below code is the way it is, possibly including a description of the binary layout in 'key'. PS1, Line 364: Pair<List<Integer>, PartialRow> lower = decodePartitionKey(schema, partitionSchema, lowerBound); : Pair<List<Integer>, PartialRow> upper = decodePartitionKey(schema, partitionSchema, upperBound); Maybe some sort of assertion that lower and upper agree on hash buckets? Alternatively, don't bother decoding the hash buckets of the upper bound. 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? 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 com.google.common.base.Preconditions; Where is this used? 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.info(KuduScanToken.stringifySerializedToken(token.serialize(), syncClient)); 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
