[kudu-CR] KUDU-1563. Add support for INSERT IGNORE

["Brock Noland (Code Review)" <ge...@cloudera.org>]

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 <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes