David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1563. Add support for INSERT IGNORE

Patch Set 7:


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

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 

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?).

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

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.

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

Reply via email to