Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3868 to look at the new patch set (#2). 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 --- 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, 110 insertions(+), 61 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/2 -- To view, visit http://gerrit.cloudera.org:8080/3868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org>