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<KuduTable>& 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 <[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 02:18:16 +0000 Gerrit-HasComments: Yes
