Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 23: (9 comments) > Patch Set 20: > > (13 comments) Thanks for reviewing! http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2474 PS20, Line 2474: // The initial status should > Should this initialize as INVALID_QUERY_HANDLE? What if this impala-server Thanks! Changed and added a new custom cluster test case for this. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2480 PS20, Line 2480: unique_ptr<ExecutorGroup> other_coordinators{ExecutorGroup::GetFilteredExecut > nit: I think it is possible to send the RPC asynchronously to all coordinat Thanks! Checking. Will reply later. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2494 PS20, Line 2494: } : KillQueryResponsePB response; : const int num_retries = 3; > Please double check how this will interact with query_timeout_s and fetch_r Thanks! Checking. Will reply later. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2500 PS20, Line 2500: request, &response, quer > Should this move on to the next Coordinator on error? query_id might live i I think when 'rpc_status' is an error, it means we failed to send the request or to receive the response. In this case, it might be hard to know whether the erroring coordinator is the coordinator of the query or not. Would it be better to return the error back than to ignore it? What do you think? Thanks! http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2505 PS20, Line 2505: status = Status(resp > Please LOG a message if the Coordinator is found (here and L2509). Thanks! Added. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/impala-server.cc@1793 PS20, Line 1793: const TUniqueId& query_id, bool from_kill_query_stateme > Add information whether the request comes from KILL query or client RPC. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/impala-server.cc@1812 PS20, Line 1812: ("User '$0' is not autho > Add parameter to say where the cancel request is coming from: KILL query, o Thanks! Added. http://gerrit.cloudera.org:8080/#/c/21930/17/tests/util/cancel_util.py File tests/util/cancel_util.py: http://gerrit.cloudera.org:8080/#/c/21930/17/tests/util/cancel_util.py@23 PS17, Line 23: from tests.common.test_result_verifier import error_msg_expected > I think cluster_config.py content should merge into custom_cluster_test_sui Thanks! I think putting the cluster configs in a separate file can make it easier for the reader to figure out what they are, because we can use them like ``` @cluster_config.xxx ``` What do you think? Thanks! http://gerrit.cloudera.org:8080/#/c/21930/20/tests/util/cancel_util.py File tests/util/cancel_util.py: http://gerrit.cloudera.org:8080/#/c/21930/20/tests/util/cancel_util.py@104 PS20, Line 104: assert not (join_before_close and use_kill_query_statement) : : if table_format: ImpalaTestSuite.change_database(client, table_format) : if exec_ > Write this as a constraint in TestCancellation. Thanks! Changed. -- To view, visit http://gerrit.cloudera.org:8080/21930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If12d6e47b256b034ec444f17c7890aa3b40481c0 Gerrit-Change-Number: 21930 Gerrit-PatchSet: 23 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Fri, 03 Jan 2025 09:43:44 +0000 Gerrit-HasComments: Yes
