Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21803 )
Change subject: IMPALA-915: Support cancel queries in frontend ...................................................................... Patch Set 29: (4 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 have another RuntimeException propagating in CatalogD. I wish CancelledException just extend ImpalaException (which is a checked exception) and we handle the propagation properly. Is it possible to break this patch to two part? 1. Implement TCancelResponse Cancel(1: TCancelRequest req); just in CatalogD code, and then JUnit test CatalogServiceCatalog for cancellation (see `git grep CatalogServiceCatalog fe/src/test`). 2. Fully implement cancellation from coordinator all the way to CatalogD. 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: ? Is it because header is usually the second field everywhere else? 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? 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: CancelledException I think "QueryCancelledException" or "UserCancelledException" is more descriptive. -- 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 <[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: Thu, 06 Feb 2025 18:52:56 +0000 Gerrit-HasComments: Yes
