Alexey Serbin has posted comments on this change. Change subject: C++ client: deprecating KuduPartialRow::SetString() ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/3723/3//COMMIT_MSG Commit Message: Line 21: An alternative approach might be to change behavior of > That would break ABI compatibility and it doesn't seem to be really necessa OK, for the other approach a created a separate changeset: http://gerrit.cloudera.org:8080/3868 http://gerrit.cloudera.org:8080/#/c/3723/3/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: Line 289: Status KuduPartialRow::SetBinaryNoCopy(const Slice& col_name, const Slice& val) { > style note: If you change the order in the header file please also change t Done http://gerrit.cloudera.org:8080/#/c/3723/3/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: Line 98: // Set the string/binary value but does not copy the value. The slice > style nit: It makes more sense to me to keep all the string methods togethe Good point! However, the point here is to emphasize the groups of different behavior. Line 109: ATTRIBUTE_DEPRECATED( > style nit: Line continuations should be 4 spaces, not 2. See https://google Done Line 110: "use SetStringNoCopy() instead; also consider using SetStringCopy()"); > style: Another line continuation, add an additional 4 spaces more than the Done -- To view, visit http://gerrit.cloudera.org:8080/3723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
