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 43: (4 comments) http://gerrit.cloudera.org:8080/#/c/21803/43//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21803/43//COMMIT_MSG@19 PS43, Line 19: Updates locks to lockInteruptibly. Provides an : implementation of getUninterruptibly that allows cancellation. Is this still relevant? I don't see changes about this in the patch? http://gerrit.cloudera.org:8080/#/c/21803/43//COMMIT_MSG@52 PS43, Line 52: Testing: Please also run tests/query_test/test_cancellation.py exhaustively. Note that most of our test use HS2 protocol by default now. So you might want to run another test_cancellation.py with env var DEFAULT_TEST_PROTOCOL="beeswax". http://gerrit.cloudera.org:8080/#/c/21803/43/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21803/43/be/src/service/client-request-state.cc@290 PS43, Line 290: // Don't start executing the query if Cancel() was called between planning and Exec(). : RETURN_IF_CANCELLED(this); This and other places where RETURN_IF_CANCELLED newly added will obtain lock_ Please make sure that they still adhere to the lock acquisition order. /// Locking /// ------- /// This class is partially thread-safe. To ensure freedom from deadlock, if multiple /// locks are acquired, lower-numbered locks must be acquired before higher-numbered /// locks: /// 1. session_state_map_lock_ /// 2. query_expiration_lock_ /// 3. SessionState::lock /// 4. idle_query_statuses_lock_ /// 5. ClientRequestState::fetch_rows_lock /// 6. ClientRequestState::lock /// 7. ClientRequestState::expiration_data_lock_ /// 8. Coordinator::exec_summary_lock /// 9. ClientRequestState::exec_state_lock_ http://gerrit.cloudera.org:8080/#/c/21803/43/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/43/fe/src/main/java/org/apache/impala/service/Canceller.java@44 PS43, Line 44: // Registers threads by their query ID so the threads can be interrupted on : // cancellation. : private static ConcurrentHashMap<TUniqueId, Thread> : queryThreads_ = new ConcurrentHashMap<>(); : // Registers that the specified thread has been cancelled and should clean up. : private static Set<Thread> cancelledThreads_ = ConcurrentHashMap.newKeySet(); This make sense. But I wonder if they should just be wrapped into a class with two synchronized method: protected synchronized unregister(TUniqueId queryId); protected synchronized cancel(TUniqueId queryId); That way, mutual exclusivity is ensured during unregister vs cancel. -- 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: 43 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, 05 Jun 2025 20:03:52 +0000 Gerrit-HasComments: Yes
