Brock Noland has posted comments on this change.

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

Patch Set 2:


Still have work todo, but wanted to wanted to get these comments out of my 
File python/kudu/

PS2, Line 18: fr
> python client needs a test?
Yep as noted in my WIP commit message.
File src/kudu/client/

Line 1830:     ASSERT_TRUE(insert == nullptr) << "Successful insert should take 
> I don't this this assertion is doing anything useful. Whether or not the in
Yeah too be honest, I just copied that portion of the assertion from the test 
directly above.

Should I remove it?

Line 1842:     // INSERT IGNORE does not result in error on duplicate
> nit" rephrase this comment a bit better?

Line 1848:     vector<string> rows;
> anyway to consolidate the duplicated code below
File src/kudu/common/

Line 130:       case 9:
> curious is this actually being used somewhere in the test?
I think the point of this test is to just execute the Add() method?
File src/kudu/common/

Line 313:                                                     const uint8_t* 
> incorrect wrapping. see
File src/kudu/common/wire_protocol.proto:

Line 173:     // Used inserting a row and ignoring any duplicate row errors
> missing a word?

Line 174:     INSERT_IGNORE = 10;
> can you move this next to the plain types at the beginning, but still keep 
File src/kudu/tablet/local_tablet_writer.h:

Line 66:   Status InsertIgnore(const KuduPartialRow& row) {
> is this called anywhere?
Should be when I finish the tests.
File src/kudu/tablet/row_op.h:

Line 44:   void SetInsertIgnoreSucceeded();
> the impl of this does nothing but resetting the OperationResultPB, do we re
If we don't reset to OperationResultPB, I get a seg fault. This part I don't 
really understand so I'd be very very open to suggests or even just 
clarifications if:

1. I am doing the right thing.
2. If the members of OperationResultPB aren't set, what happens?
File src/kudu/tablet/tablet.h:

Line 402:   Status InsertOrInsertIgnoreOrUpsertUnlocked(WriteTransactionState 
> I'm torn whether we should change the name of this function to add the igno
I did this but didn't like it. I am going to revert.
File src/kudu/tablet/tablet_metrics.h:

PS2, Line 50: scoped_refptr<Counter> rows_insert_ignored;
> do we really need this in tablet metrics? what would be the use?
Yeah I can see folding this into rows_Inserted. At the same time, I like more 
transparency. If you feel strongly I can remove.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland <>
Gerrit-Reviewer: Brock Noland <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

Reply via email to