You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2016/11/08 01:22:51 UTC

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2 

Matthew Jacobs has uploaded a new change for review.

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

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................

IMPALA-3710: Kudu DML should ignore conflicts, pt2

Second part of IMPALA-3710, which removed the IGNORE DML
option and changed the following errors on Kudu DML
operations to be ignored:
* INSERT where the PK already exists
* UPDATE/DELETE where the PK doesn't exist

This changes other data-related errors to be ignored as
well:
* NULLs in non-nullable columns, i.e. null constraint
  violoations.
* Rows with PKs that are in an 'uncovered range'.

This also adds some metadata to the thrift report sent to
the coordinator from sinks so the total number of rows with
errors can be added to the profile.

An upcoming change will add this new info to the beeswax
interface and show it in the shell output (IMPALA-3713).

Testing: Updated kudu_crud tests to check the number of rows
with errors.

Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
---
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/runtime/coordinator.cc
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
9 files changed, 174 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4985/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


IMPALA-3710: Kudu DML should ignore conflicts, pt2

Second part of IMPALA-3710, which removed the IGNORE DML
option and changed the following errors on Kudu DML
operations to be ignored:
1) INSERT where the PK already exists
2) UPDATE/DELETE where the PK doesn't exist

This changes other data-related errors to be ignored as
well:
3) NULLs in non-nullable columns, i.e. null constraint
  violoations.
4) Rows with PKs that are in an 'uncovered range'.

It became clear that we can't differentiate between (3) and
(4) because both return a Kudu 'NotFound' error code. The
Impala error codes have been simplified as well: we just
report a generic KUDU_NOT_FOUND error in these cases.

This also adds some metadata to the thrift report sent to
the coordinator from sinks so the total number of rows with
errors can be added to the profile. Note that this does not
include a breakdown of error counts by type/code because we
cannot differentiate between all of these cases yet.

An upcoming change will add this new info to the beeswax
interface and show it in the shell output (IMPALA-3713).

Testing: Updated kudu_crud tests to check the number of rows
with errors.

Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Reviewed-on: http://gerrit.cloudera.org:8080/4985
Reviewed-by: Alex Behm <al...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/runtime/coordinator.cc
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
9 files changed, 180 insertions(+), 101 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Alex Behm: Looks good to me, but someone else must approve
  Dan Hecht: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4985/1/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

Line 119:     if (!dst_stats->__isset.kudu_stats) dst_stats->__set_kudu_stats(TKuduDmlStats());
pass 0 in c'tor of TKuduDmlStats?


Line 161:       ss << "NumRowErrors: " << stats.kudu_stats.num_row_errors << endl;
General question: Is there an easy way to distinguish the different types of errors? We have PK constraint violation, nullability violation, and partition not found.

I'm not sure how useful a consolidate error count will be to users. It's hard to tell what errors exactly were hit.

Seems weird to have different error codes, but then aggregate all of them here in the profile into a single counter.


http://gerrit.cloudera.org:8080/#/c/4985/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

Line 214:           COUNTER_ADD(num_row_errors_, 1);
This is expensive right? How about we update a local counter and COUNTER_ADD() at the end of this loop, just like we do for total_rows.


Line 305:     if (!e.IsNotFound() && !e.IsAlreadyPresent() && status.ok()) {
Why not move to L325 into an else if?


Line 310:       // Kudu does not have yet have a way to programmatically differentiate between 'row
few words mixed up


http://gerrit.cloudera.org:8080/#/c/4985/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 430: // TODO: Refactor to reflect usage by other DML statements.
Do you intend to do that soon? The structures are a little confusing now.


Line 445:   // The latest observed Kudu timestamp reported by the KuduSession at this partition.
is partition == table here?


-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4985/1/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

Line 161:       ss << "NumRowErrors: " << stats.kudu_stats.num_row_errors << endl;
> Agree that misinformation is worse. Can you add a comment to the counter si
I put a comment in the Thrift definition of num_row_errors which I thought would be more visible. I can move it here if you think that's better?


http://gerrit.cloudera.org:8080/#/c/4985/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 430: // TODO: Refactor to reflect usage by other DML statements.
> I think it makes sense to do this non-functional rename/refactoring in a se
Ok I agree, I'll do it separately


Line 445:   // The latest observed Kudu timestamp reported by the KuduSession at this partition.
> reported by the local KuduSession?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 3: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has uploaded a new patch set (#2).

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................

IMPALA-3710: Kudu DML should ignore conflicts, pt2

Second part of IMPALA-3710, which removed the IGNORE DML
option and changed the following errors on Kudu DML
operations to be ignored:
1) INSERT where the PK already exists
2) UPDATE/DELETE where the PK doesn't exist

This changes other data-related errors to be ignored as
well:
3) NULLs in non-nullable columns, i.e. null constraint
  violoations.
4) Rows with PKs that are in an 'uncovered range'.

It became clear that we can't differentiate between (3) and
(4) because both return a Kudu 'NotFound' error code. The
Impala error codes have been simplified as well: we just
report a generic KUDU_NOT_FOUND error in these cases.

This also adds some metadata to the thrift report sent to
the coordinator from sinks so the total number of rows with
errors can be added to the profile. Note that this does not
include a breakdown of error counts by type/code because we
cannot differentiate between all of these cases yet.

An upcoming change will add this new info to the beeswax
interface and show it in the shell output (IMPALA-3713).

Testing: Updated kudu_crud tests to check the number of rows
with errors.

Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
---
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/runtime/coordinator.cc
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
9 files changed, 179 insertions(+), 101 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4985/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4985/2/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS2, Line 188: violoations
spelling


http://gerrit.cloudera.org:8080/#/c/4985/2/be/src/exec/kudu-table-sink.h
File be/src/exec/kudu-table-sink.h:

PS2, Line 119: rows
maybe "rows consumed"? or "rows processed"?


-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4985/1/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

Line 161:       ss << "NumRowErrors: " << stats.kudu_stats.num_row_errors << endl;
> It is unfortunate and probably not helpful enough, but I'm very worried abo
Agree that misinformation is worse. Can you add a comment to the counter similar to what you responded here? Just as an explanation why all errors are purposely aggregated.


http://gerrit.cloudera.org:8080/#/c/4985/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 430: // TODO: Refactor to reflect usage by other DML statements.
> Yeah I'll go ahead and do this if you think the strategy is otherwise OK.
I think it makes sense to do this non-functional rename/refactoring in a separate patch.


Line 445:   // The latest observed Kudu timestamp reported by the KuduSession at this partition.
> Hm yeah I guess it's not great to call this a partition... Table I think mi
reported by the local KuduSession?


-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 1:

BTW, based on a conversation with the Kudu folks I learned my handling of the 'NotFound' errors wasn't right. I changed the code to not attempt to break them apart.

-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 3: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4985/2/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS2, Line 188: violoations
> spelling
Done


http://gerrit.cloudera.org:8080/#/c/4985/2/be/src/exec/kudu-table-sink.h
File be/src/exec/kudu-table-sink.h:

PS2, Line 119: rows
> maybe "rows consumed"? or "rows processed"?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4985/1/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

Line 119:     if (!dst_stats->__isset.kudu_stats) dst_stats->__set_kudu_stats(TKuduDmlStats());
> pass 0 in c'tor of TKuduDmlStats?
I can't bc there's no parameter, I think because I use optional fields.


Line 161:       ss << "NumRowErrors: " << stats.kudu_stats.num_row_errors << endl;
> General question: Is there an easy way to distinguish the different types o
It is unfortunate and probably not helpful enough, but I'm very worried about us presenting incorrect information to the user which I think is worse. The issue is that we can't trust the Kudu error codes enough. E.g. they re-use error codes for some different cases where I'm not confident enough that we'll always get it right. My goal is to work with them for the next release to get a more concrete contract. I was on the fence about having different Impala error codes at all, but I think it's worthwhile for de-duping the logs. If you think this is confusing, I'd prefer to just remove the warning logging w/ different error codes.


http://gerrit.cloudera.org:8080/#/c/4985/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

Line 214:           COUNTER_ADD(num_row_errors_, 1);
> This is expensive right? How about we update a local counter and COUNTER_AD
Done


Line 305:     if (!e.IsNotFound() && !e.IsAlreadyPresent() && status.ok()) {
> Why not move to L325 into an else if?
Done


Line 310:       // Kudu does not have yet have a way to programmatically differentiate between 'row
> few words mixed up
Done


http://gerrit.cloudera.org:8080/#/c/4985/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 430: // TODO: Refactor to reflect usage by other DML statements.
> Do you intend to do that soon? The structures are a little confusing now.
Yeah I'll go ahead and do this if you think the strategy is otherwise OK.


Line 445:   // The latest observed Kudu timestamp reported by the KuduSession at this partition.
> is partition == table here?
Hm yeah I guess it's not great to call this a partition... Table I think might be confusing too since there are many of these inserting across the cluster for the same table. How about I just remove 'at this partition'?


-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3710: Kudu DML should ignore conflicts, pt2

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has uploaded a new patch set (#3).

Change subject: IMPALA-3710: Kudu DML should ignore conflicts, pt2
......................................................................

IMPALA-3710: Kudu DML should ignore conflicts, pt2

Second part of IMPALA-3710, which removed the IGNORE DML
option and changed the following errors on Kudu DML
operations to be ignored:
1) INSERT where the PK already exists
2) UPDATE/DELETE where the PK doesn't exist

This changes other data-related errors to be ignored as
well:
3) NULLs in non-nullable columns, i.e. null constraint
  violoations.
4) Rows with PKs that are in an 'uncovered range'.

It became clear that we can't differentiate between (3) and
(4) because both return a Kudu 'NotFound' error code. The
Impala error codes have been simplified as well: we just
report a generic KUDU_NOT_FOUND error in these cases.

This also adds some metadata to the thrift report sent to
the coordinator from sinks so the total number of rows with
errors can be added to the profile. Note that this does not
include a breakdown of error counts by type/code because we
cannot differentiate between all of these cases yet.

An upcoming change will add this new info to the beeswax
interface and show it in the shell output (IMPALA-3713).

Testing: Updated kudu_crud tests to check the number of rows
with errors.

Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
---
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/runtime/coordinator.cc
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
9 files changed, 180 insertions(+), 101 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4985/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eb1ad91dc355ea51de261c3a14df0f9d28c879c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>