Mike Percy has posted comments on this change. Change subject: C++ client: deprecating KuduPartialRow::SetString() ......................................................................
Patch Set 3: (5 comments) Feel free to hold off on doing reordering stuff until we have consensus on the ABI issue. Then again, if both Todd and Dan are away next week then whoever is being responsive on Gerrit probably wins... :) http://gerrit.cloudera.org:8080/#/c/3723/3//COMMIT_MSG Commit Message: Line 21: An alternative approach might be to change behavior of > I also like that approach (that was the initial idea I had), but after some That would break ABI compatibility and it doesn't seem to be really necessary in this case. I think we should take ABI breaks very seriously and only do it when we must. Also if we are to follow semver then we need to bump the major version to do this. That said, I don't know of anybody other than Impala actually using the C++ API in the data path, so if we were going to do it then we need to do it before 1.0 My personal preference is the current approach though 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 the order in the cc file to match 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 together and then keep all the binary methods together. Line 109: ATTRIBUTE_DEPRECATED( style nit: Line continuations should be 4 spaces, not 2. See https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions Line 110: "use SetStringNoCopy() instead; also consider using SetStringCopy()"); style: Another line continuation, add an additional 4 spaces more than the previous line is indented per https://google.github.io/styleguide/cppguide.html#Function_Calls -- 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
