Todd Lipcon has posted comments on this change. Change subject: KUDU-1563. Add support for INSERT IGNORE ......................................................................
Patch Set 8: (12 comments) What happens if you try to INSERT IGNORE against an older tserver which doesn't support it? I'm guessing it results in some kind of nasty error, and we should add a feature flag to the RPC that gets set if any INSERT IGNORE is used. http://gerrit.cloudera.org:8080/#/c/4491/8//COMMIT_MSG Commit Message: PS8, Line 9: Add's nit: adds http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS8, Line 1847: insertIgnore nit (here and elsewhere below): should be 'insert_ignore', not camelCase in C++ http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/client/write_op.h File src/kudu/client/write_op.h: PS8, Line 151: insert ignore capitalize INSERT IGNORE http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/common/row_operations-test.cc File src/kudu/common/row_operations-test.cc: Line 129: enc.Add(RowOperationsPB::INCLUSIVE_RANGE_UPPER_BOUND, row); missing a 'break' here. http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: Line 319: ops->push_back({TEST_INSERT_IGNORE, row_key}); nit: funny indentation Line 321: ops_pending = false; ops_pending should be true either way (since there is something to flush) Line 322: data_in_mrs = false; I don't think you should reset data_in_mrs, because it won't _remove_ data if there is any there Line 324: exists[row_key] = true; you could just set this unconditionally http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/tablet/tablet-test-base.h File src/kudu/tablet/tablet-test-base.h: PS8, Line 328: int64_t count, : int32_t val, : TimeSeries *ts = NULL) { : nit: indent http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: PS8, Line 408: default seems it would be safer to explicitly do a 'case INSERT' here, and then have default: be a LOG(FATAL) Line 438: default: same http://gerrit.cloudera.org:8080/#/c/4491/8/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: PS8, Line 34: insert ignored > Maybe this should be counting the number of errors that were ignored instea yea, this isn't a very clear description. I agree the count should be the number of duplicate rows which were ignored due to the "INSERT IGNORE' flag, rather than "the number of insert attempts with ignore" (to match the same idea that 'rows_inserted' only counts those successfully inserted) -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Brock Noland <br...@phdata.io> Gerrit-Reviewer: Brock Noland <br...@phdata.io> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
