[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[Brock Noland (Code Review)]

Brock Noland has posted comments on this change.

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


Patch Set 2:

(1 comment)

I am going to push a new series of patches. Still a WIP though.

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";
> Yeah too be honest, I just copied that portion of the assertion from the te
Done


-- 
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 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[Brock Noland (Code Review)]

Brock Noland has posted comments on this change.

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


Patch Set 2:

(2 comments)

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) {
> seems like it's time to have an enum to replace these bools
We are already comparing against and enum so I just removed these flags and use 
the enum directly.


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.
Done


-- 
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 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 2:

(1 comment)

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 27: public class InsertIgnore extends Operation {
As part of this patch or in a separate one, we should deprecate 
KuduSession#setIgnoreAllDuplicateRows and all that's related to it and tell 
folks to use InsertIgnore if they really want to ignore row errors. It used to 
be useful to set that parameter when we didn't have the replay cache but now 
it's not really an issue anymore.


-- 
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 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[Brock Noland (Code Review)]

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 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 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 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[David Ribeiro Alves (Code Review)]

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

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

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


Patch Set 2:

At first glance I'd start by suggesting you split the patches for the various 
clients. Different people are experts in the different clients and you'll get 
parallel reviews and better and quicker feedback

-- 
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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

[Brock Noland (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4491

to look at the new patch set (#2).

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

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

Add's `INSERT IGNORE' operation which behaves like a
normal `INSERT' except in the case when a duplicate
row error would be raised by the primary key having been
previously inserted.

Follows upsert backend/c++ patch 56c431585ed7ad07ef and
java patch 591c9ccb2a08bcfa4c.

The part I am most unsure about is returning an empty
OperationResultPB as I am not really sure where and how
the members of that operation are used. Spelunking didn't
clarify that for my newb eyes.

Also need some more tests. The python change is completely
untested.

Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
---
A java/kudu-client/src/main/java/org/apache/kudu/client/InsertIgnore.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java
M python/kudu/__init__.py
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/wire_protocol.proto
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/transactions/transaction.cc
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/write_transaction.cc
27 files changed, 322 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4491/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot