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 <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to