David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1563. Add support for INSERT IGNORE ......................................................................
Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: Line 80: using client::KuduColumnSchema; > warning: using decl 'KuduColumnSchema' is unused [misc-unused-using-decls] not your fault, but could you address these warnings? hard to keep track of the new ones versus the old ones. you can leave out the TODO(username) ones that are not introduced by this patch. Line 411: LOG(FATAL) << "r = " << r; can you add more info to the output? i.e. what is "r". the row key might also be helpful http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet-test-base.h File src/kudu/tablet/tablet-test-base.h: Line 326: // Inserts "count" rows. add: "... , ignoring errors in the case of duplicate keys." or something like that. PS7, Line 354: if (i % 2 == 0) { : CHECK_OK(writer.Insert(row)); : } else { : CHECK_OK(writer.InsertIgnore(row)); : } : } not sure about this block. What are you trying to do here. You've clearly already added a new op type. Are you trying to make it so that most tests, which use regular inserts, also test the insert ignore code path? I like the idea, but not sure this is the best way to go. doesn't this make some tests fail (and if not shouldn't it?). http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: Line 401: if (present) { would still rather have a switch here (this was one of the intentions of the original comment, though I now notice I didn't mention it at all, guess you didn't read my mind :) ). Line 407: op->SetInsertIgnoreSucceeded(); this method name here reads even weirder. How about: op->SetInsertErrorIgnored() or something http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 365: // refer to INSERT IGNORE. It refers to inserts ignored during log replay add: "... purposefully ignored due to duplicate key errors during log replay" or something like that. http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: PS2, Line 50: scoped_refptr<Counter> rows_insert_ignored; > Yeah I can see folding this into rows_Inserted. At the same time, I like mo how about renaming the var to something like: insert_errors_ignored. or something of the kind -- 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: 7 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-HasComments: Yes
