Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18536 )
Change subject: IMPALA-10465: Use IGNORE variant of Kudu write operations ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/18536/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18536/5//COMMIT_MSG@22 PS5, Line 22: 9 iterations > No particular reason. Just pick an odd number out of habit to get exact med Ah, I see. http://gerrit.cloudera.org:8080/#/c/18536/5//COMMIT_MSG@34 PS5, Line 34: Additionally, this patch adds 'workload_iterations' option in : single_node_perf_run.py to loop a workload and modify DDL_CRUD_PATTERN : in query_executor.py to not not trigger EXPLAIN in regular CREATE : query (non CTAS). > The kudu-ignore.test that I craft require these changes in Impala perf scri Ah, maybe those changes then could go first before this patch, so this patch would be based on that one? http://gerrit.cloudera.org:8080/#/c/18536/5/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/18536/5/be/src/exec/kudu-table-sink.cc@254 PS5, Line 254: if (sink_action_ == TSinkAction::INSERT) { : return table_->NewInsertIgnore(); : } else if (sink_action_ == TSinkAction::UPDATE) { : return table_->NewUpdateIgnore(); : } else if (sink_action_ == TSinkAction::UPSERT) { : return table_->NewUpsert(); : } else { : DCHECK(sink_action_ == TSinkAction::DELETE) : << "Sink type not supported: " << sink_action_; : return table_->NewDeleteIgnore(); : } > This is implemented with 'if/else' just to maintain consistency with the ex Yeah: makes sense to keep this uniform, indeed. http://gerrit.cloudera.org:8080/#/c/18536/5/be/src/exec/kudu-table-sink.cc@356 PS5, Line 356: // Since ignore_conflicts_ is true, there should be no Kudu error message. : DCHECK(session_->CountPendingErrors() == 0); > My assumption is that XXX_IGNORE ops will not return any error message back IIRC, there might be any mix of errors in a Kudu session. For example, since the session talks simultaneously with many tablet servers, it might happen that one tablet server returns stats for duplicated keys for INSERT_IGNORE from one server, but timeout from other server. Or there might be some column constraint violation reported for one row while other rows report on ignore operation metrics. http://gerrit.cloudera.org:8080/#/c/18536/5/be/src/exec/kudu-table-sink.cc@360 PS5, Line 360: if (is_transactional_) { : // Return general status error to abort transaction. : return Status("Kudu reported write operation errors during transaction."); : } > This is to maintain previous behavior. In line 382, Impala immediately retu Right, but in line 382 there could be no errors due to INSERT_IGNORE operations since those were not supported, right? If so, then I'm not sure what sort of previous behavior we are trying to maintain here. http://gerrit.cloudera.org:8080/#/c/18536/5/be/src/exec/kudu-table-sink.cc@364 PS5, Line 364: return Status::OK(); > Impala does not mix write operation in single KuduTableSink instance lifeti All right, UPSERT is out of question here but what about other session errors, if any? Something about timeouts, column constraint violations, etc.? http://gerrit.cloudera.org:8080/#/c/18536/5/fe/src/main/java/org/apache/impala/planner/KuduTableSink.java File fe/src/main/java/org/apache/impala/planner/KuduTableSink.java: http://gerrit.cloudera.org:8080/#/c/18536/5/fe/src/main/java/org/apache/impala/planner/KuduTableSink.java@75 PS5, Line 75: supportsIgnoreOperations_ = client.supportsIgnoreOperations(); > I think we can put an ExecOption info in Impala query profile about this. SGTM! -- To view, visit http://gerrit.cloudera.org:8080/18536 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8da7c41d61b0888378b390b8b643238433eb3b52 Gerrit-Change-Number: 18536 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Sat, 21 May 2022 00:16:01 +0000 Gerrit-HasComments: Yes
