Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17144 )
Change subject: IMPALA-10551: Add result sink support for external frontends ...................................................................... Patch Set 7: (1 comment) Apart from the interaction with retry_failed_queries, this looks good to me. If you want to split off the retry_failed_queries piece into a separate change, I am ok with that. I think this change is ok if the external frontend guarantees not to pass in retry_failed_queries=true when using a result sink. http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc@802 PS6, Line 802: RETURN_IF_ERROR(UpdateExecState(Status::OK(), nullptr, FLAGS_hostname)); > I agree that retry_failed_queries might be problematic and we might want to The retrying happens at a level above the Coordinator object (at the QueryDriver level). Each new retry would have its own ClientRequestState object (which has its own Coordinator object). Here is a comment covering some of the concepts: https://github.com/apache/impala/blob/master/be/src/runtime/query-driver.h#L75 The pieces of code in Coordinator that would trigger a retry are the TryQueryRetry() calls in this file. I believe we did not intend for the retries to apply to DMLs, but I tried it out and we do actually retry DMLs right now. So, at the moment, there is nothing disabling retries for something with a result sink. I will file a JIRA for disabling retry_failed_queries for DMLs. I think that was unintentional. -- To view, visit http://gerrit.cloudera.org:8080/17144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I024bf41d77bb81f1ab0debdbd31ec3687c83f072 Gerrit-Change-Number: 17144 Gerrit-PatchSet: 7 Gerrit-Owner: John Sherman <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 15 Mar 2021 18:49:10 +0000 Gerrit-HasComments: Yes
