Brock Noland has posted comments on this change. Change subject: [WIP] KUDU-1563. Add support for INSERT IGNORE ......................................................................
Patch Set 2: (12 comments) Still have work todo, but wanted to wanted to get these comments out of my buffer. 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? Yep as noted in my WIP commit message. 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 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? Done Line 1848: vector<string> rows; > anyway to consolidate the duplicated code below Done 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? I think the point of this test is to just execute the Add() method? 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#F Done 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? Done Line 174: INSERT_IGNORE = 10; > can you move this next to the plain types at the beginning, but still keep Done 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? Should be when I finish the tests. 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 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? 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, > 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. 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? 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 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: Brock Noland <br...@phdata.io> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
