Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22422 )
Change subject: IMPALA-13703: Cancel running queries before shutdown deadline ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/runtime/query-exec-mgr.cc@288 PS7, Line 288: }, > We could maybe consolidate the logic for `CancelQueriesForGracefulShutdown` Done http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/runtime/query-exec-mgr.cc@303 PS7, Line 303: CancelQueries(cancellation_task); > In the graceful shutdown case we're not calling 'ReleaseQueryState'? Is tha ReleaseQueryState() is called in QueryExecMgr::CancelQueries(). Calling ReleaseQueryState() alone may use an async way for cancellation, as noted in the comment: "which will get cancelled eventually anyway when QueryState::ReportExecStatus() hits the timeout." However, in the shutdown case, may be more controlled way to use the current shutdown thread to retry doing the whole thing after one second if queue is full. http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/service/cancellation-work.h File be/src/service/cancellation-work.h: http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/service/cancellation-work.h@32 PS7, Line 32: // The query is being terminated because a backend failed. We can skip cancelling the : // query if the fragment instances running on that backend all completed. : BACKEND_FAILED, : // The Impala Server terminated the query during shutdown and the query has not been : // done yet when the graceful shutdown deadline is reached. : GRACEFUL_SHUTDOWN > I see that `do_test_shutdown_executor` provides coverage for this scenario. Added one more testcase test_shutdown_coordinator_and_executor_cancel_query. http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/22422/7/be/src/service/impala-server.h@1676 PS7, Line 1676: /// running queries have been sent for cancellation. > I think I see that we're using this atomic to retry query cancellation in t Yeah, it ensures to be true only when all running queries have been sent for cancellation. Added some comments. -- To view, visit http://gerrit.cloudera.org:8080/22422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cac2e100d329644e21fdceb0b23901b08079130 Gerrit-Change-Number: 22422 Gerrit-PatchSet: 8 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 05 Feb 2025 11:40:22 +0000 Gerrit-HasComments: Yes
