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 30: (14 comments) http://gerrit.cloudera.org:8080/#/c/21803/29//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21803/29//COMMIT_MSG@25 PS29, Line 25: CancelledException is implemented as a RuntimeException for the moment : because exception propagation requires a lot of updates; this is similar : to LocalCatalogException, and both should be cleaned up. > RuntimeException is difficult to reason about, and it makes me worry to hav Implementing proper exception propagation in catalogd is the bulk of new work. Is that the part you're suggesting put in (1)? I went a ways down that route, but after changing several hundred function signatures across dozens of files it wasn't clear there was an end in sight. 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? ExpireQuery's 3rd argument is whether to also call UnregisterQuery. In this patch, I relabeled "idled queries" to "interrupted queries", and moved handling status retention (this block) to UnregisterQuery when the unregister happens due to an "interruption". Idle query cancellation became part of that classification. http://gerrit.cloudera.org:8080/#/c/21803/29/be/src/service/impala-server.cc@1586 PS29, Line 1586: mmar > nit: might be more readable to add a parameter comment, i.e. /* skip_rpcs * Done http://gerrit.cloudera.org:8080/#/c/21803/29/be/src/service/impala-server.cc@1775 PS29, Line 1775: return shared_ptr<QueryDriver>(); > Why have this if statement? To me, it makes more sense to return the error I think it was because I'd extracted it from GetActiveQueryHandle, and I wanted to have consistent logging. But I think I've moved away from using other methods to get query handles, so I could pull it back out to only happen in GetActiveQueryHandle. 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@596 PS29, Line 596: : // Common header included in all CatalogService requests. : 6: optional TCatalogServiceRequestHeader header > Why this is put here instead of the last after 5: ? Yes, it's because header is usually the second field. This was also done in TGetCatalogObjectRequest and TGetFunctionsRequest. http://gerrit.cloudera.org:8080/#/c/21803/29/common/thrift/CatalogService.thrift@780 PS29, Line 780: 2: required Types.TUniqueId query_id > Should this be TCatalogServiceRequestHeader? I suppose it could be. In this case we're acting directly on this field, so header feels a bit opaque. And it's not exactly a catalog request. 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: loader, pool.submit(() -> { loader.load(); return null; }))); : } > It'd be nice if we can cancel the FileMetadataLoader threads as well since Won't that get handled by the pool.shutdown? I may have considered changing that to shutdownNow, but was worried about the side-effects. http://gerrit.cloudera.org:8080/#/c/21803/29/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/29/fe/src/main/java/org/apache/impala/service/Canceller.java@54 PS29, Line 54: push > The name "put" is typically associated with a stack data type. I suggest r Done http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/Canceller.java@54 PS29, Line 54: TUniqueId queryId > I recommend making this a java.util.Optional<TUniqueId> instead of using nu I'm not sure I see the value. http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/Canceller.java@63 PS29, Line 63: pop > The name "pop" is typically associated with a stack data type. I suggest r Done 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 l Ack 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 mod Ack http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/21803/29/fe/src/main/java/org/apache/impala/service/Frontend.java@2041 PS29, Line 2041: public static void cancelExecRequest(TUniqueId queryId) { > At first I didn't see the need for this function but then realized its used I think it doesn't have a point anymore actually, JniFrontend could invoke this directly. http://gerrit.cloudera.org:8080/#/c/21803/29/tests/util/web_pages_util.py File tests/util/web_pages_util.py: http://gerrit.cloudera.org:8080/#/c/21803/29/tests/util/web_pages_util.py@81 PS29, Line 81: _ > Note: 22452 is now merged 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: 30 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: Wed, 12 Feb 2025 22:43:52 +0000 Gerrit-HasComments: Yes
