You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Brock Noland (Code Review)" <ge...@cloudera.org> on 2016/11/28 02:42:42 UTC

[kudu-CR] Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Brock Noland has uploaded a new change for review.

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

Change subject: Implement feature flag support on WriteRpc and add flag for INSERT IGNORE
......................................................................

Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
6 files changed, 54 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/5241/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5241
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland <br...@phdata.io>

[kudu-CR] WIP KUDU-1563. Implement feature flag support IGNORE operations

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.

Grant Henke has uploaded a new patch set (#8) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/5241 )

Change subject: WIP KUDU-1563. Implement feature flag support IGNORE operations
......................................................................

WIP KUDU-1563. Implement feature flag support IGNORE operations

This patch adds a tablet server feature flag to indicate that
the server supports `IGNORE` operations. This includes
INSERT_IGNORE, DELETE_IGNORE, and UPDATE_IGNORE.

Note: This patch isn’t required, but provides a slightly better error
and message. Without this patch ops that are unsupported return a
`Corruption` status with the message “Unknown operation type”.

WIP: This shouldn’t be comitted until all of the operations are
comitted.

Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
4 files changed, 43 insertions(+), 3 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/5241/8
--
To view, visit http://gerrit.cloudera.org:8080/5241
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 8
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Posted by "Brock Noland (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, 

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

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

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

Change subject: Implement feature flag support on WriteRpc and add flag for INSERT IGNORE
......................................................................

Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
7 files changed, 53 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/5241/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5241
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 4
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] WIP KUDU-1563. Implement feature flag support IGNORE operations

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/5241 )

Change subject: WIP KUDU-1563. Implement feature flag support IGNORE operations
......................................................................


Patch Set 9:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/batcher.cc@275
PS9, Line 275:   // The set of server features required by this rpc
             :   std::unordered_set<uint32_t> required_server_features_;
This is certainly future-proof for additional feature flags, but given that writes are a hot path, perhaps we'd be better served with a simple boolean for IGNORE_OPERATIONS?


http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/batcher.cc@338
PS9, Line 338:         required_server_features_.insert(tserver::TabletServerFeatures::IGNORE_OPERATIONS);
Inserting to a hash-based set on every op is inefficient; it's a hash and comparison even when the set already contains the desired value. How about setting some local bool to true, then outside of the loop, doing just one insert to the set if the bool tests true?


http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/client-test.cc@2488
PS9, Line 2488: TEST_F(ClientTest, TestRequiredServerFeatures) {
Indentation is off, should be 2 char not 4 char.


http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/tserver/tablet_service.cc@164
PS9, Line 164: service
Nit: prefix with tserver instead of service.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 9
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 18 Feb 2020 01:34:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Posted by "Brock Noland (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, 

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

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

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

Change subject: Implement feature flag support on WriteRpc and add flag for INSERT IGNORE
......................................................................

Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
6 files changed, 52 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/5241/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5241
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 2
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Posted by "Brock Noland (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, 

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

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

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

Change subject: Implement feature flag support on WriteRpc and add flag for INSERT IGNORE
......................................................................

Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
6 files changed, 52 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 5
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] WIP KUDU-1563. Implement feature flag support IGNORE operations

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.

Grant Henke has uploaded a new patch set (#9) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/5241 )

Change subject: WIP KUDU-1563. Implement feature flag support IGNORE operations
......................................................................

WIP KUDU-1563. Implement feature flag support IGNORE operations

This patch adds a tablet server feature flag to indicate that
the server supports `IGNORE` operations. This includes
INSERT_IGNORE, DELETE_IGNORE, and UPDATE_IGNORE.

WIP: This shouldn’t be comitted until all of the operations are
comitted.

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 9
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Posted by "Brock Noland (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, 

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

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

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

Change subject: Implement feature flag support on WriteRpc and add flag for INSERT IGNORE
......................................................................

Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
6 files changed, 52 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/5241/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5241
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 3
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] Implement feature flag support on WriteRpc and add flag for INSERT IGNORE

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/5241 )

Change subject: Implement feature flag support on WriteRpc and add flag for INSERT IGNORE
......................................................................


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/client-test.cc@2440
PS7, Line 2440:   ASSERT_FALSE(session->HasPendingOperations());
Seems unnecessary; you started an empty session on L2439.


http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/client-test.cc@2441
PS7, Line 2441:   session->SetTimeoutMillis(10000);
Why is this necessary?


http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/client-test.cc@2455
PS7, Line 2455: 
Nit: remove empty line


http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:

http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/write_op.h@119
PS7, Line 119:   std::set<uint32_t> required_server_features_;
KuduWriteOperation (and its children) are part of the public API, and adding a new data member like this breaks ABI compatibility [1]. It's unfortunate that KuduWriteOperation and friends weren't PIMPL'd, but I think that was done because they're used en masse, so avoiding an extra heap allocation is beneficial.

That's even before we talk about the overhead of an std::set per class instance. Could we somehow push this to the KuduSession, which is PIMPL'd and is less performance sensitive?

1. https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B for more details.


http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/client/write_op.h@199
PS7, Line 199: 
Nit: unintended addition?


http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/5241/7/src/kudu/tserver/tablet_service.cc@1618
PS7, Line 1618:     case TabletServerFeatures::INSERT_IGNORE: return FLAGS_service_inject_support_insert_ignore;
Could wrap in PREDICT_TRUE().



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b
Gerrit-Change-Number: 5241
Gerrit-PatchSet: 7
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 03 Dec 2018 23:50:24 +0000
Gerrit-HasComments: Yes