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 <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
