[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/4491 )
Change subject: KUDU-1563. Add an INSERT_IGNORE operation
..
Patch Set 20:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/4491/20//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/4491/20//COMMIT_MSG@13
PS20, Line 13: In the future
> So referring back to my discussion with Brock [1], what are your expectatio
The plan is to start with ignoring duplicate/missing PKs for all IGNORE
operations and then later on add session or batch configuration if more types
of "ignore" toggles are required.
I expect if/when we add other types of errors to ignore that duplicate/missing
PKs can still be the default but each error type but can be enabled and
disabled the same as any other config.
It has become clear that ignoring PK errors is by far the most frequently
requested and expected thing to ignore. To the point we have implemented some
bad workarounds in integrations. We don't have workaround for the other error
types yet.
My expected steps are:
- Add INSERT_IGNORE (duplicate PK only)
- Add DELETE_INGORE (missing PK only)
- Add UPDATE_IGNORE (missing PK only)
- Add feature handling for backwards compatibility
- Update workaround implementations to use new ops when the server supports it
(Backup, Spark, Nifi)
- Document the new IGNORE ops
- File Jira about optionally ignoring other errors with the IGNORE operations
and wait to see demand/need.
Do you think I need to share a design doc on the mailing list and get
consensus? Given this is strictly an improvement on the bad client workarounds
already committed, I though it was safe to proceed.
http://gerrit.cloudera.org:8080/#/c/4491/20//COMMIT_MSG@13
PS20, Line 13: ingored
> ignored
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@356
PS20, Line 356: void InsertTestRows(KuduTable* table, KuduSession* session,
> warning: method 'InsertTestRows' can be made static [readability-convert-me
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@408
PS20, Line 408: return std::move(insert);
> warning: moving a local object in a return statement prevents copy elision
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@408
PS20, Line 408: return std::move(insert);
> clang-tidy is correct here and below; if you just return the object without
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@415
PS20, Line 415: return std::move(insert);
> warning: moving a local object in a return statement prevents copy elision
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@2429
PS20, Line 2429: static void DoTestInsertIgnoreVerifyRows(const
shared_ptr& tbl, int num_rows) {
> 2 char indentation.
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@2450
PS20, Line 2450: ASSERT_FALSE(session->HasPendingOperations()) << "Should
not have pending operation";
> I don't see why you need to test this if you're using AUTO_FLUSH_SYNC; don'
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/write_op.h@167
PS20, Line 167: INSERT IGNORE
> INSERT IGNORE? insert ignore? The function doc has both. "insert ignore" is
Done
http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/client/write_op.h@a187
PS17, Line 187:
> Nit: unintended delete? We separate classes by two lines in this file.
Done
http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/client/write_op.h@163
PS17, Line 163: };
> Nit: add an extra line here.
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/integration-tests/fuzz-itest.cc@107
PS20, Line 107: TEST_INSERT_IGNORE,
> Maybe add a PK_ONLY variant too?
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/integration-tests/fuzz-itest.cc@740
PS20, Line 740: ops->push_back({TEST_INSERT_IGNORE, row_key});
> ops->emplace_back(r, row_key);
Done
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/row_op.h@103
PS20, Line 103:
: // True if an INSERT_IGNORE op was ignored
: bool insert_error_ignored = false;
> Define this with the other bool members, to avoid extra padding added by th
Done
http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/tablet/row_op.h
File
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has uploaded a new patch set (#22) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/4491 ) Change subject: KUDU-1563. Add an INSERT_IGNORE operation .. KUDU-1563. Add an INSERT_IGNORE operation This patch adds an `INSERT_IGNORE' operation which behaves like a normal `INSERT' except in the case when a duplicate row error would be raised by the primary key having been previously inserted. In the future if more types of errors need to be ignored, other than key errors, we can add session or batch level configurations to set which errors should be ignored. I didn’t do that in this patch to keep things simple and becase ignoring key errors is the only use-case I have seen required/requested. At that time UPSERT_INGORE could be added as well. Follow on patches will add more client support. Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metrics.cc M src/kudu/tablet/tablet_metrics.h M src/kudu/tablet/tablet_random_access-test.cc M src/kudu/tablet/transactions/transaction.cc M src/kudu/tablet/transactions/transaction.h M src/kudu/tablet/transactions/write_transaction.cc 24 files changed, 344 insertions(+), 72 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4491/22 -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 22 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has uploaded a new patch set (#21) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/4491 ) Change subject: KUDU-1563. Add an INSERT_IGNORE operation .. KUDU-1563. Add an INSERT_IGNORE operation This patch adds an `INSERT_IGNORE' operation which behaves like a normal `INSERT' except in the case when a duplicate row error would be raised by the primary key having been previously inserted. In the future if more types of errors need to be ignored, other than key errors, we can add session or batch level configurations to set which errors should be ignored. I didn’t do that in this patch to keep things simple and becase ignoring key errors is the only use-case I have seen required/requested. At that time UPSERT_INGORE could be added as well. Follow on patches will add more client support. Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metrics.cc M src/kudu/tablet/tablet_metrics.h M src/kudu/tablet/tablet_random_access-test.cc M src/kudu/tablet/transactions/transaction.cc M src/kudu/tablet/transactions/transaction.h M src/kudu/tablet/transactions/write_transaction.cc 24 files changed, 353 insertions(+), 72 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4491/21 -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 21 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [test] fix flake in TsTabletManagerITest::TestTableStats
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15224 to look at the new patch set (#5). Change subject: [test] fix flake in TsTabletManagerITest::TestTableStats .. [test] fix flake in TsTabletManagerITest::TestTableStats Sometimes the leader replica of a tablet can change its role prior to receiving StepDown request, so the test outputs somelike like: I0214 18:13:07.095886 5028 raft_consensus.cc:572] T a32ee1063340424db8a7e7ea6aedcfa1 P ddfa34be67fd4a40b26218185403580c [term 3 FOLLOWER]: Rejecting request to step down while not leader src/kudu/integration-tests/ts_tablet_manager-itest.cc:818: Failure Failed Bad status: Illegal state: Code NOT_THE_LEADER: Not currently leader To avoid test flakiness it makes sense to continue with next tablet when observing such an error. To verify the fix, I ran the scenario in TSAN configuration with and without this patch: before: 16 NOT_THE_LEADER failures in 256 runs after: 0 NOT_THE_LEADER failures in 256 runs Note, however: even after one source of flakiness is gone, this scenario is still very flaky. In addition, I revised other tests where LeaderStepDown() might fail due to fluctuating leadership and updated them accordingly. Change-Id: Ibbd6652cdcb30f8899767dfb932cc402cf739115 --- M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/raft_consensus_election-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/tombstoned_voting-stress-test.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc 5 files changed, 75 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/15224/5 -- To view, visit http://gerrit.cloudera.org:8080/15224 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibbd6652cdcb30f8899767dfb932cc402cf739115 Gerrit-Change-Number: 15224 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [test] fix flake in TsTabletManagerITest::TestTableStats
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15224 ) Change subject: [test] fix flake in TsTabletManagerITest::TestTableStats .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc: http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1100 PS4, Line 1100: !s.ok() && > Don't need. Done http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/ts_tablet_manager-itest.cc@824 PS4, Line 824: !s.ok() && s.IsIllegalState() > This is duplicative; the test on s.IsIllegalState() is sufficient. Done http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/ts_tablet_manager-itest.cc@828 PS4, Line 828: continue; > Do we want to CheckStats here too? Or do so after waiting for kMaxElectionT If the idea of this scope is checking stats after each election, probably we could do it. However, the idea behind this patch was simply to skip the rare case when the behavior is not of the expected one. From that perspective, I don't think calling CheckStats() is necessary here. -- To view, visit http://gerrit.cloudera.org:8080/15224 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibbd6652cdcb30f8899767dfb932cc402cf739115 Gerrit-Change-Number: 15224 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 18 Feb 2020 03:52:00 + Gerrit-HasComments: Yes
[kudu-CR] [python] KUDU-1563. Add support for INSERT IGNORE
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/4522 ) Change subject: [python] KUDU-1563. Add support for INSERT_IGNORE .. Patch Set 20: (2 comments) http://gerrit.cloudera.org:8080/#/c/4522/20/python/kudu/client.pyx File python/kudu/client.pyx: http://gerrit.cloudera.org:8080/#/c/4522/20/python/kudu/client.pyx@846 PS20, Line 846: def new_insert_ignore(self, record): Should we allow record to be None as for the other new_*** functions? http://gerrit.cloudera.org:8080/#/c/4522/20/python/kudu/client.pyx@848 PS20, Line 848: Create a new Insert Ignore operation. Pass the completed Insert Ignore to a Session. Seems like there's more docs that can be cribbed from the other new_*** functions. -- To view, visit http://gerrit.cloudera.org:8080/4522 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c45a50d4b87d8f7c4f0f83fbc72932d056d3a79 Gerrit-Change-Number: 4522 Gerrit-PatchSet: 20 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 18 Feb 2020 02:21:32 + Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-1563. Add support for INSERT IGNORE
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/4523 ) Change subject: [java] KUDU-1563. Add support for INSERT_IGNORE .. Patch Set 25: (3 comments) http://gerrit.cloudera.org:8080/#/c/4523/25//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/4523/25//COMMIT_MSG@11 PS25, Line 11: I manually tested this against an old server version without the INSERT_IGNORE operation suport and it returns the correct Too long. Also 'support', and I think there are two spaces in a row between 'operation' and 'support'. http://gerrit.cloudera.org:8080/#/c/4523/25/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/4523/25/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@171 PS25, Line 171: maybe may be http://gerrit.cloudera.org:8080/#/c/4523/25/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@170 PS25, Line 170: An insert ignore, ignores :* duplicate row errors How about "An insert ignore will ignore duplicate row errors". -- To view, visit http://gerrit.cloudera.org:8080/4523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd Gerrit-Change-Number: 4523 Gerrit-PatchSet: 25 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 18 Feb 2020 02:19:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/4491 )
Change subject: KUDU-1563. Add an INSERT_IGNORE operation
..
Patch Set 20:
(11 comments)
Thanks for picking this up.
Please see my earlier feedback, some of it is still valid.
http://gerrit.cloudera.org:8080/#/c/4491/20//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/4491/20//COMMIT_MSG@13
PS20, Line 13: ingored
ignored
http://gerrit.cloudera.org:8080/#/c/4491/20//COMMIT_MSG@13
PS20, Line 13: In the future
So referring back to my discussion with Brock [1], what are your expectations
going forward re: INSERT IGNORE? Will it only ever ignore duplicate PKs, and if
there's a desire to ignore additional errors on insert, new operation types
must be added? I'm asking because we can define the scope of the INSERT IGNORE
contract now in such a way that we have wiggle room should the details change
in the future.
1. https://gerrit.cloudera.org/c/4491/17//COMMIT_MSG#12
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@408
PS20, Line 408: return std::move(insert);
> warning: moving a local object in a return statement prevents copy elision
clang-tidy is correct here and below; if you just return the object without
std::move, you'll get the copy elision you want.
https://stackoverflow.com/questions/12953127/what-are-copy-elision-and-return-value-optimization
has much more detail.
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@2429
PS20, Line 2429: static void DoTestInsertIgnoreVerifyRows(const
shared_ptr& tbl, int num_rows) {
2 char indentation.
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/client-test.cc@2450
PS20, Line 2450: ASSERT_FALSE(session->HasPendingOperations()) << "Should
not have pending operation";
I don't see why you need to test this if you're using AUTO_FLUSH_SYNC; don't we
guarantee that Apply() flushes the operation?
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/client/write_op.h@167
PS20, Line 167: INSERT IGNORE
INSERT IGNORE? insert ignore? The function doc has both. "insert ignore" is
probably better seeing as we call a KuduInsert an "insert" rather than an
"INSERT".
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/integration-tests/fuzz-itest.cc@107
PS20, Line 107: TEST_INSERT_IGNORE,
Maybe add a PK_ONLY variant too?
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/integration-tests/fuzz-itest.cc@740
PS20, Line 740: ops->push_back({TEST_INSERT_IGNORE, row_key});
ops->emplace_back(r, row_key);
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/row_op.h@103
PS20, Line 103:
: // True if an INSERT_IGNORE op was ignored
: bool insert_error_ignored = false;
Define this with the other bool members, to avoid extra padding added by the
compiler to RowOp.
See https://en.wikipedia.org/wiki/Data_structure_alignment for much more detail.
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/tablet-test.cc
File src/kudu/tablet/tablet-test.cc:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/tablet-test.cc@752
PS20, Line 752: TYPED_TEST(TestTablet, TestInsertIgnore) {
Nit: 2 char indentation.
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/tablet_metrics.cc
File src/kudu/tablet/tablet_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/4491/20/src/kudu/tablet/tablet_metrics.cc@34
PS20, Line 34: METRIC_DEFINE_counter(tablet, insert_errors_ignored, "Insert
Errors Ignored",
Indentation of continuation lines should be 4 chars.
Also maybe "Insert Ignore Errors" to keep "insert" and "ignore" together,
indicating INSERT IGNORE?
--
To view, visit http://gerrit.cloudera.org:8080/4491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-Change-Number: 4491
Gerrit-PatchSet: 20
Gerrit-Owner: Brock Noland
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Brock Noland
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 18 Feb 2020 02:18:16 +
Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1563. Implement feature flag support IGNORE operations
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 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 18 Feb 2020 01:34:07 +
Gerrit-HasComments: Yes
[kudu-CR] [test] fix flake in TsTabletManagerITest::TestTableStats
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15224 ) Change subject: [test] fix flake in TsTabletManagerITest::TestTableStats .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc: http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1100 PS4, Line 1100: !s.ok() && Don't need. http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/ts_tablet_manager-itest.cc@824 PS4, Line 824: !s.ok() && s.IsIllegalState() This is duplicative; the test on s.IsIllegalState() is sufficient. http://gerrit.cloudera.org:8080/#/c/15224/4/src/kudu/integration-tests/ts_tablet_manager-itest.cc@828 PS4, Line 828: continue; Do we want to CheckStats here too? Or do so after waiting for kMaxElectionTime (which I presume we do to allow for a new leader to be elected somewhere)? -- To view, visit http://gerrit.cloudera.org:8080/15224 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibbd6652cdcb30f8899767dfb932cc402cf739115 Gerrit-Change-Number: 15224 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 18 Feb 2020 01:27:47 + Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-1563. Add support for INSERT IGNORE
Grant Henke has removed a vote on this change. Change subject: [java] KUDU-1563. Add support for INSERT_IGNORE .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/4523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd Gerrit-Change-Number: 4523 Gerrit-PatchSet: 25 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] KUDU-1563. Add support for INSERT IGNORE
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/4523 ) Change subject: [java] KUDU-1563. Add support for INSERT_IGNORE .. Patch Set 25: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd Gerrit-Change-Number: 4523 Gerrit-PatchSet: 25 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 17 Feb 2020 20:51:32 + Gerrit-HasComments: No
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/4491 ) Change subject: KUDU-1563. Add an INSERT_IGNORE operation .. Patch Set 20: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 20 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 17 Feb 2020 20:23:52 + Gerrit-HasComments: No
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has removed a vote on this change. Change subject: KUDU-1563. Add an INSERT_IGNORE operation .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 20 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP KUDU-1563. Implement feature flag support IGNORE operations
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. 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, 42 insertions(+), 3 deletions(-) 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [java] KUDU-1563. Add support for INSERT IGNORE
Grant Henke has uploaded a new patch set (#25) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/4523 ) Change subject: [java] KUDU-1563. Add support for INSERT_IGNORE .. [java] KUDU-1563. Add support for INSERT_IGNORE Implements java support for the `INSERT_IGNORE' operation. I manually tested this against an old server version without the INSERT_IGNORE operation suport and it returns the correct error with a message that says: “Unknown operation type: 10”. Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd --- A java/kudu-client/src/main/java/org/apache/kudu/client/InsertIgnore.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java 4 files changed, 126 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/4523/25 -- To view, visit http://gerrit.cloudera.org:8080/4523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd Gerrit-Change-Number: 4523 Gerrit-PatchSet: 25 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has uploaded a new patch set (#20) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/4491 ) Change subject: KUDU-1563. Add an INSERT_IGNORE operation .. KUDU-1563. Add an INSERT_IGNORE operation This patch adds an `INSERT_IGNORE' operation which behaves like a normal `INSERT' except in the case when a duplicate row error would be raised by the primary key having been previously inserted. In the future if more types of errors need to be ingored, other than key errors, we can add session or batch level configurations to set which errors should be ignored. I didn’t do that in this patch to keep things simple and becase ignoring key errors is the only use-case I have seen required/requested. At that time UPSERT_INGORE could be added as well. Follow on patches will add more client support. Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metrics.cc M src/kudu/tablet/tablet_metrics.h M src/kudu/tablet/tablet_random_access-test.cc M src/kudu/tablet/transactions/transaction.cc M src/kudu/tablet/transactions/transaction.h M src/kudu/tablet/transactions/write_transaction.cc 24 files changed, 337 insertions(+), 70 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4491/20 -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 20 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1563. Add an INSERT IGNORE operation
Grant Henke has uploaded a new patch set (#19) to the change originally created by Brock Noland. ( http://gerrit.cloudera.org:8080/4491 ) Change subject: KUDU-1563. Add an INSERT_IGNORE operation .. KUDU-1563. Add an INSERT_IGNORE operation This patch adds an `INSERT_IGNORE' operation which behaves like a normal `INSERT' except in the case when a duplicate row error would be raised by the primary key having been previously inserted. In the future if more types of errors need to be ingored, other than key errors, we can add session or batch level configurations to set which errors should be ignored. I didn’t do that in this patch to keep things simple and becase ignoring key errors is the only use-case I have seen required/requested. At that time UPSERT_INGORE could be added as well. Follow on patches will add more client support. Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metrics.cc M src/kudu/tablet/tablet_metrics.h M src/kudu/tablet/tablet_random_access-test.cc M src/kudu/tablet/transactions/transaction.cc M src/kudu/tablet/transactions/transaction.h M src/kudu/tablet/transactions/write_transaction.cc 24 files changed, 336 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4491/19 -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 19 Gerrit-Owner: Brock Noland Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brock Noland Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tools] KUDU-2413 Configurable # of reactors in loadgen
Attila Bukor has abandoned this change. ( http://gerrit.cloudera.org:8080/10542 ) Change subject: [tools] KUDU-2413 Configurable # of reactors in loadgen .. Abandoned doesn't seem to be necessary after all -- To view, visit http://gerrit.cloudera.org:8080/10542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I8f02e6ac153ff58c2d3a89a9966ea6fe721db06e Gerrit-Change-Number: 10542 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2972 Add Ranger client
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15206 to look at the new patch set (#8). Change subject: KUDU-2972 Add Ranger client .. KUDU-2972 Add Ranger client Adds a Ranger client that communicates with the Java subprocess that wraps the Ranger plugin. It can be used by an authorization provider to translate between Kudu and Ranger. Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 --- M src/kudu/common/table_util-test.cc M src/kudu/common/table_util.cc M src/kudu/common/table_util.h M src/kudu/ranger/CMakeLists.txt A src/kudu/ranger/ranger_action.cc A src/kudu/ranger/ranger_action.h A src/kudu/ranger/ranger_client-test.cc A src/kudu/ranger/ranger_client.cc A src/kudu/ranger/ranger_client.h M src/kudu/subprocess/server.cc M src/kudu/subprocess/server.h M src/kudu/util/subprocess.cc M src/kudu/util/subprocess.h 13 files changed, 639 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/8 -- To view, visit http://gerrit.cloudera.org:8080/15206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 Gerrit-Change-Number: 15206 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
