Todd Lipcon has posted comments on this change. Change subject: KUDU-842. Implement Operation.toString() in Java client ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/1945/2/java/kudu-client/src/main/java/org/kududb/client/RowKeyStringifier.java File java/kudu-client/src/main/java/org/kududb/client/RowKeyStringifier.java: Line 35: public static String stringifyRowKey(Operation op) { why not just make this a member function of PartialRow, since you don't use the Operation at all? Then it will just be a simple change to support stringifying the whole row (which is useful anyway) Line 46: ColumnSchema col = schema.getColumnByIndex(i); should probably add an assertion that col is not nullable and a precondition check that it has been set in the partialrow. Otherwise if you create an Operation, and haven't yet set the key, and you call toString() on it, you'll get some random garbage Line 55: value.reset(); shouldn't the return value of getVarLengthData() already be pointed at the beginning of the data? why is reset necessary? http://gerrit.cloudera.org:8080/#/c/1945/2/java/kudu-client/src/test/java/org/kududb/client/TestOperation.java File java/kudu-client/src/test/java/org/kududb/client/TestOperation.java: Line 134: public void testRowKeyStringify() { add a test for stringifying a partially-set key -- To view, visit http://gerrit.cloudera.org:8080/1945 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia258bc9bd4140ae0085bebe51935b465dac43db9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
