Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21803 )
Change subject: IMPALA-915: Support cancel queries in frontend ...................................................................... Patch Set 51: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/21803/51/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/51/fe/src/main/java/org/apache/impala/service/Canceller.java@127 PS51, Line 127: private synchronized boolean isCancelled() { : return cancelledThreads_.contains(Thread.currentThread()); : } Why not use a ThreadLocal AtomicBoolean to store whether a planner Thread is cancelled and add it to queryThreads_? The benefit is that isCancelled() checks wouldn't need to take a global lock. This would be especially useful if someone would add more points to analysis/planning where cancellation can be checked, so isCancelled could be called a large number of time. http://gerrit.cloudera.org:8080/#/c/21803/51/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/21803/51/tests/webserver/test_web_pages.py@1129 PS51, Line 1129: text_profile_url = "{0}query_profile_plain_text?query_id={1}".format(self.ROOT_URL : .format("25000"), query_id) : response = requests.get(text_profile_url) This is not new code in the patch, but I think that it would be better t have a function for this in ImpalaTestSuite (e.g. get_coordinator_web_page()), similarly to https://github.com/apache/impala/blob/384a664f54f6fa32dc39d3368ad19fda225bff01/tests/common/impala_test_suite.py#L620 hardcoding the port here and using requests directly makes it harder to port tests to other environments -- 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: 51 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@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: Fri, 20 Jun 2025 08:23:29 +0000 Gerrit-HasComments: Yes
