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 34: (12 comments) http://gerrit.cloudera.org:8080/#/c/21803/32/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21803/32/be/src/catalog/catalog-server.cc@469 PS32, Line 469: VLOG_RPC << "Cancel(): request=" << ThriftDebugString(req); > VLOG_RPC is VLOG(2). I think Cancellation is important enough to be at leas This is cancelling the catalog request, not the original Cancellation request. It seemed more in line with other catalog RPCs. The same logic might apply to ExecDdl. http://gerrit.cloudera.org:8080/#/c/21803/29/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21803/29/be/src/service/impala-server.cc@1775 PS29, Line 1775: return shared_ptr<QueryDriver>(); > I think it was because I'd extracted it from GetActiveQueryHandle, and I wa Oh, I forgot I added an additional argument and only passed it in there. I've move this out of the function. http://gerrit.cloudera.org:8080/#/c/21803/29/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/21803/29/common/thrift/CatalogService.thrift@780 PS29, Line 780: > Thanks. Btw, Quanlong is adding one in separate patch. That's in Frontend.java, while TCancelRequests originate from C++ code. But it turns out there's a ClientRequestState helper I can use. http://gerrit.cloudera.org:8080/#/c/21803/32/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/32/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3859 PS32, Line 3859: try (Canceller.Registration _ = Canceller.register(queryId)) { : ret > Can use try-with-resources pattern? Done http://gerrit.cloudera.org:8080/#/c/21803/29/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/29/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@576 PS29, Line 576: ValueType ret = Canceller.getUninterruptibly(existing); > If the original query that starts the request is cancelled, the current que I haven't encountered that case. http://gerrit.cloudera.org:8080/#/c/21803/29/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/29/fe/src/main/java/org/apache/impala/common/CancelledException.java@23 PS29, Line 23: > I think "QueryCancelledException" or "UserCancelledException" is more descr Done http://gerrit.cloudera.org:8080/#/c/21803/32/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/32/fe/src/main/java/org/apache/impala/service/Canceller.java@51 PS32, Line 51: public static class Registration implements AutoCloseable { : private TUniqueId queryId_; : : public Registration(TUniqueId queryId) { : queryId_ = queryId; : } > put and remove sounds like a container method. Done http://gerrit.cloudera.org:8080/#/c/21803/32/fe/src/main/java/org/apache/impala/service/Canceller.java@83 PS32, Line 83: // 3. cancel() puts to cancelledThreads_ > Or maybe, queryThreads_.get(queryId) first, synchronized over the returned We'd need to check queryThreads_.get again after synchronized, in case remove was called between them. Both queryThreads_ and cancelledThreads_ are concurrent data structures, and there's a pretty clear order to these. We wouldn't be able to avoid them being concurrent data structures, so I don't see the value in adding additional synchronization overhead. http://gerrit.cloudera.org:8080/#/c/21803/32/fe/src/main/java/org/apache/impala/service/Canceller.java@95 PS32, Line 95: if (queryId == null) return; : Thread queryThread = queryThreads_.get(queryId); > What happen if this code is reached? Is it indicate bugs? It would usually represent that another CANCEL was requested while the first one was finishing. I could add some debug actions so we can write a test for it. http://gerrit.cloudera.org:8080/#/c/21803/32/fe/src/test/java/org/apache/impala/common/QueryFixture.java File fe/src/test/java/org/apache/impala/common/QueryFixture.java: http://gerrit.cloudera.org:8080/#/c/21803/32/fe/src/test/java/org/apache/impala/common/QueryFixture.java@109 PS32, Line 109: catch (UserCancelledExcep > This is the only place I can see that catch CancelledException. That would primarily be in individual Jni interface methods. I'm not sure I have a comprehensive list, and it'd be easy to miss some. http://gerrit.cloudera.org:8080/#/c/21803/29/tests/custom_cluster/test_web_pages.py File tests/custom_cluster/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/21803/29/tests/custom_cluster/test_web_pages.py@553 PS29, Line 553: @CustomClusterTestSuite.with_args( > Should there be a test that runs a query that spawns child queries to ensur I think the child query would spawn after planning has finished. So it's unrelated to this patch. I'm not sure that we have a test covering cancelling queries with a child query though, so could be worth following up on. http://gerrit.cloudera.org:8080/#/c/21803/32/tests/custom_cluster/test_web_pages.py File tests/custom_cluster/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/21803/32/tests/custom_cluster/test_web_pages.py@719 PS32, Line 719: # Cancel refresh query > I think what we are missing the coverage is cancelling the first query in t Done -- 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: 34 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-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 18 Feb 2025 23:25:48 +0000 Gerrit-HasComments: Yes
