Mike Percy has submitted this change and it was merged.
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 a safe modification, but it is not backward-compatible
in semver notation since it changes the semantics
of already existing methods. However, we don't bump API version
since Kudu is not 1.0 yet. As for the ABI, it remains compatible
with the prior versions: the existing client C++ code
will still compile and link.
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
at 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
Reviewed-on: http://gerrit.cloudera.org:8080/3868
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mpe...@apache.org>
---
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, 266 insertions(+), 72 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 12
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
