Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3868
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
......................................................................
[KuduPartialRow::Set{Binary,String}()] copy input data
KuduPartialRow::SetBinary()/SetString() behavior was optimized
to omit copying of the passed data. However, a user of the API
might assume these methods 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, the behavior of these methods has been changed
to immediately copy the input data.
This is safe, but an incompatible change, but we don't bump
API/ABI version since Kudu is not 1.0 yet.
This approach may add some performance regression issues for
already existing clients, but hopefully it is negligible for most
of the C++ clients around. As for the Impala-Kudu integration,
it seems current Impala code uses SetString() only in one place
in be/src/sec/kudu-table-sink.cc, and that can be addressed
separately.
An alternative approach might be to deprecate
KuduPartialRow::Set{Binary,String}() in favor of the newly introduced
KuduPartialRow::Set{Binary,String}NoCopy() methods. That approach
would spare us from performance regression worries and compatibility
issues. However, after some discussion, introducing the copying
behavior of KuduPartialRow::SetBinary()/SetString() methods
appeared to be more beneficial in the long run
from the usability perspective.
Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/stubs.h
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/gutil/port.h
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
14 files changed, 108 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/1
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
