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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
