Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21803 )
Change subject: IMPALA-915: Support cancel queries in frontend ...................................................................... Patch Set 25: (11 comments) http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/runtime/query-driver.cc File be/src/runtime/query-driver.cc: http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/runtime/query-driver.cc@a94 PS24, Line 94: I explained in the commit message: > Removes setting UpdateQueryStatus when GetExecRequest returns because that's > already handled in ImpalaServer::Execute when it calls UnregisterQuery in > response to an error, and constitutes an update race on the status with > UnregisterQuery triggered by CancelQueryHandler. We want to use the status > from CancelQueryHandler in this case as it provides more context (about who > initiated the cancel); the result of GetExecRequest is just > CancelledException. http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/service/client-request-state.cc@246 PS24, Line 246: RETURN_IF_CANCELLED(this); > Nit: A comment here about how the client request state got into cancelled m Will do. http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/service/client-request-state.cc@1155 PS24, Line 1155: RETURN_IF_CANCELLED(this); > Nit: A comment here about how the client request state got into cancelled m I'll check if this is still needed. Normal control flow may be sufficient to handle this case. http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/21803/24/be/src/service/fe-support.cc@592 PS24, Line 592: if (!request.__isset.header) { > If the request header is set, does that guarantee the header's query_id wil I haven't seen a case where it's needed. http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/catalog/CatalogCancelledException.java File fe/src/main/java/org/apache/impala/catalog/CatalogCancelledException.java: http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/catalog/CatalogCancelledException.java@26 PS24, Line 26: public class CatalogCancelledException extends CatalogException { > How easy is it to identify the query id for which this exception is thrown? The query ID is a prefix to the log entry. http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3825 PS24, Line 3825: throw new CatalogException("Error running getPartialCatalogObject(): ", e); > Can we improve this when the InterruptedException is thrown at L3796? You mean improve the error message we produce? It will include the original exception, which points to it being interrupted by the user. http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@575 PS24, Line 575: isPiggybacked = true; : Future<ValueType> existing = (Future<ValueType>)inCache; : ValueType ret = Uninterruptibles.getUninterruptibly(existing); > The piggyback mechanism makes different queries be able to share getPartial So another request would be blocked on the future? Yes, I can change this to not use getUninterruptibly (or use a modified version that can be cancelled). As a meta question, I'm not sure there's value in creating separate CancelledException rather than using InterruptedException. There are lots of cases where we ignore InterruptedException, and I'm not sure if they're because other things can trigger an interrupt or because no one wanted to deal with the exception propagation. http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/common/CancelledException.java File fe/src/main/java/org/apache/impala/common/CancelledException.java: http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/common/CancelledException.java@23 PS24, Line 23: public class CancelledException extends ImpalaException { > How easy is it to identify the query id for which this exception is thrown? The query ID is a prefix to the log entry. http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/service/Canceller.java File fe/src/main/java/org/apache/impala/service/Canceller.java: http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/service/Canceller.java@42 PS24, Line 42: threads generating TExecRequest > nit: now we have threads handling catalogd RPCs as well Will do. http://gerrit.cloudera.org:8080/#/c/21803/24/fe/src/main/java/org/apache/impala/service/Canceller.java@47 PS24, Line 47: // TODO: use thread-local storage > Will this todo be resolved before this change is submitted? If not, please Yeah, working on it. http://gerrit.cloudera.org:8080/#/c/21803/24/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/21803/24/tests/shell/test_shell_interactive.py@333 PS24, Line 333: def test_cancellation(self, vector): > Can we add tests for impala-shell? It seems we can't cancel the query in CR Yeah, makes sense. I'll need to look into why Ctrl+C doesn't work, but probably related to using CancelInternal instead of UnregisterQuery. -- To view, visit http://gerrit.cloudera.org:8080/21803 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0d25d4c7fb0b8dcc7dad9510db1e8dca220eeb86 Gerrit-Change-Number: 21803 Gerrit-PatchSet: 25 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 16 Dec 2024 23:00:04 +0000 Gerrit-HasComments: Yes
