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<KuduTable>& 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 src/kudu/tablet/row_op.h: http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/tablet/row_op.h@107 PS17, Line 107: > Move this up to the other bools, so that less padding is generated for each Done 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. Done 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. Done -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Brock Noland <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 18 Feb 2020 04:44:27 +0000 Gerrit-HasComments: Yes
