Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21803 )
Change subject: IMPALA-915: Support cancel queries in frontend ...................................................................... Patch Set 29: (9 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: Ack 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@a2942 PS29, Line 2942: : : : : : : : : : Could you explain why we don't need these anymore? http://gerrit.cloudera.org:8080/#/c/21803/29/be/src/service/impala-server.cc@1586 PS29, Line 1586: true nit: might be more readable to add a parameter comment, i.e. /* skip_rpcs */, so we know the above comment is talking about this. http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@187 PS29, Line 187: futures.add(new Pair<>( : loader, pool.submit(() -> { loader.load(); return null; }))); It'd be nice if we can cancel the FileMetadataLoader threads as well since they are doing the real heavy work. It's OK if we want to do this in a follow-up JIRA. 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 query that wants the same result will fail since the catalogd RPC is cancelled in catalogd side. I think we need to retry in this case. http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@431 PS29, Line 431: public TDdlExecResponse execDdlRequest(TDdlExecRequest ddlRequest) We can support cancellation for DDLs in a follow-up JIRA. Some statements like ALTER TABLE RECOVER PARTITIONS might run long. http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7229 PS29, Line 7229: try { We can also support cancellation in INSERTs in a future JIRA. Before we modify any HMS partitions, e.g. after acquiring the table lock, the statement can still be cleanly cancelled. 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@632 PS29, Line 632: def test_query_cancel_piggyback(self): > What about adding a test that does the opposite of this one where it cancel +1 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): > This requires implementing IMPALA-2568 to address. The client can't issue a Maybe we can track this in IMPALA-3879. -- 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: 29 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 01:19:05 +0000 Gerrit-HasComments: Yes
