David Ribeiro Alves has posted comments on this change.

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


Patch Set 2:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/4491/2/java/kudu-client/src/main/java/org/apache/kudu/client/InsertIgnore.java
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


http://gerrit.cloudera.org:8080/#/c/4491/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java:

PS2, Line 140: Get
long line


http://gerrit.cloudera.org:8080/#/c/4491/2/python/kudu/__init__.py
File python/kudu/__init__.py:

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


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 1830:     ASSERT_TRUE(insert == nullptr) << "Successful insert should take 
ownership";
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


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:

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


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:

Line 313:                                                     const uint8_t* 
prototype_row_storage,
incorrect wrapping. see 
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/wire_protocol.proto
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


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:

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


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/row_op.h
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?


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet.cc
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


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

Line 402:   Status InsertOrInsertIgnoreOrUpsertUnlocked(WriteTransactionState 
*tx_state,
> 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?


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_bootstrap.cc
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.


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_metrics.h
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