[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Alexey Serbin (Code Review)]

Alexey Serbin has abandoned this change.

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..


Abandoned

Abandoning this in favor of http://gerrit.cloudera.org:8080/3868 which has been 
merged already.

-- 
To view, visit http://gerrit.cloudera.org:8080/3723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3723

to look at the new patch set (#11).

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..

C++ client: deprecating KuduPartialRow::SetString()

KuduPartialRow::Set{String,Binary}() are marked as deprecated.
Use KuduPartialRow::Set{String,Binary}NoCopy() instead.

KuduPartialRow::SetString()/SetBinary() behavior is optimized
to make no copies of the passed data.  However, a user of the API
might assume they are safe to use along with other setters as
SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, new methods are introduced instead:
KuduPartialRow::Set{String,Binary}NoCopy().  The new names
contain a hint about the optimized behavior of these methods.

An alternative approach might be to change behavior of
KuduPartialRow::Set{String,Binary}() and introduce only
KuduPartialRow::Set{String,Binary}NoCopy(), but there are
concerns about compatibility and performance regressions.

Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 294 insertions(+), 80 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3723/11
-- 
To view, visit http://gerrit.cloudera.org:8080/3723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..


Patch Set 11:

Build Started http://104.196.14.100/job/kudu-gerrit/2802/

-- 
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: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3723

to look at the new patch set (#9).

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..

C++ client: deprecating KuduPartialRow::SetString()

KuduPartialRow::Set{String,Binary}() are marked as deprecated.
Use KuduPartialRow::Set{String,Binary}NoCopy() instead.

KuduPartialRow::SetString()/SetBinary() behavior is optimized
to make no copies of the passed data.  However, a user of the API
might assume they are safe to use along with other setters as
SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, new methods are introduced instead:
KuduPartialRow::Set{String,Binary}NoCopy().  The new names
contain a hint about the optimized behavior of these methods.

An alternative approach might be to change behavior of
KuduPartialRow::Set{String,Binary}() and introduce only
KuduPartialRow::Set{String,Binary}NoCopy(), but there are
concerns about compatibility and performance regressions.

Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 292 insertions(+), 80 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3723/9
-- 
To view, visit http://gerrit.cloudera.org:8080/3723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/2799/

-- 
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..


Patch Set 8:

Build Started http://104.196.14.100/job/kudu-gerrit/2798/

-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3723

to look at the new patch set (#5).

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..

C++ client: deprecating KuduPartialRow::SetString()

KuduPartialRow::Set{String,Binary}() are marked as deprecated.
Use KuduPartialRow::Set{String,Binary}NoCopy() instead.

KuduPartialRow::SetString()/SetBinary() behavior is optimized
to make no copies of the passed data.  However, a user of the API
might assume they are safe to use along with other setters as
SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, new methods are introduced instead:
KuduPartialRow::Set{String,Binary}NoCopy().  The new names
contain a hint about the optimized behavior of these methods.

An alternative approach might be to change behavior of
KuduPartialRow::Set{String,Binary}() and introduce only
KuduPartialRow::Set{String,Binary}NoCopy(), but there are
concerns about compatibility and performance regressions.

Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 202 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3723/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2757/

-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: C++ client: deprecating KuduPartialRow::SetString()
..


Patch Set 3:

Can we try to close this one out for 0.10? Seems like it's basically ready and 
a good API improvement.

-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No