David Ribeiro Alves has posted comments on this change.

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

Patch Set 2:


File java/kudu-client/src/main/java/org/apache/kudu/client/InsertIgnore.java:

Line 23:  * Represents a single row insert ignoring duplicate rows. Instances 
of this class should not be reused.
long line

File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java:

PS2, Line 140: Get
long line

File python/kudu/__init__.py:

PS2, Line 18: fr
python client needs a test?

File src/kudu/client/client-test.cc:

Line 1830:     ASSERT_TRUE(insert == nullptr) << "Successful insert should take 
I don't this this assertion is doing anything useful. Whether or not the 
insertion takes ownership of the contained pointer, the container is definitely 
empty since you just called release().

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

Line 1845:     ASSERT_TRUE(insertIgnore == nullptr) << "Successful insert 
ignore should take ownership";
same as above

Line 1848:     vector<string> rows;
anyway to consolidate the duplicated code below

File src/kudu/common/row_operations-test.cc:

Line 130:       case 9:
curious is this actually being used somewhere in the test?

File src/kudu/common/row_operations.cc:

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 the 
number? you can dispense with the comment, for consistency

File src/kudu/tablet/local_tablet_writer.h:

Line 66:   Status InsertIgnore(const KuduPartialRow& row) {
is this called anywhere?

File src/kudu/tablet/row_op.h:

Line 44:   void SetInsertIgnoreSucceeded();
the impl of this does nothing but resetting the OperationResultPB, do we really 
need it?

File src/kudu/tablet/tablet.cc:

Line 406:       } else if (is_insert_ignore) {
> warning: don't use else after return [readability-else-after-return]
seems like it's time to have an enum to replace these bools

File src/kudu/tablet/tablet.h:

Line 402:   Status InsertOrInsertIgnoreOrUpsertUnlocked(WriteTransactionState 
> warning: function 'kudu::tablet::Tablet::InsertOrInsertIgnoreOrUpsertUnlock
I'm torn whether we should change the name of this function to add the ignore 
case. It's not really a different tablet operation, like upsert was, its just a 
different way to build the response for the client. thoughts?

File src/kudu/tablet/tablet_bootstrap.cc:

PS2, Line 364: Note
thanks for adding this, but might be  better to expand the comment a bit.

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?

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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

Reply via email to