Brock Noland has posted comments on this change. Change subject: KUDU-1563. Add support for INSERT IGNORE ......................................................................
Patch Set 7: (10 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; > not your fault, but could you address these warnings? hard to keep track of Done Line 82: using client::KuduInsert; > warning: using decl 'KuduInsert' is unused [misc-unused-using-decls] Done Line 83: using client::KuduInsertIgnore; > warning: using decl 'KuduInsertIgnore' is unused [misc-unused-using-decls] Done Line 411: LOG(FATAL) << "r = " << r; > can you add more info to the output? i.e. what is "r". the row key might al Done 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 li Done 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 a I'll check after rebase, but this didn't make any tests fail since this code assumes the insert will be successful, in which case the insert ignore will always be successful. However, I can see why it might be confusing. I'll re-tool. http://gerrit.cloudera.org:8080/#/c/4491/7/src/kudu/tablet/tablet-test.cc File src/kudu/tablet/tablet-test.cc: Line 598: } > Can you also test with mixed batches? Say Insert, InsertIgnore, Insert on t Yeah I was doing something similar over in tablet-test-base but I decided to change that and move into the main test. 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 th Yep gotcha, this is also better. Line 407: op->SetInsertIgnoreSucceeded(); > this method name here reads even weirder. How about: op->SetInsertErrorIgno That *is* better 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 repla Is that true though? The `inserts_ignored` property existed before insert ignore was implemented so I assumed that `inserts_ignored` means something else here but I don't know enough about log replay to know exactly what. -- 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
