Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 20: (13 comments) http://gerrit.cloudera.org:8080/#/c/21930/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21930/13//COMMIT_MSG@15 PS13, Line 15: KILL QUERY '123:456'; > Thanks! Added. Done 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: Status status = Status::OK(); Should this initialize as INVALID_QUERY_HANDLE? What if this impala-server is the only coordinator? Status status = Status::Expected(TErrorCode::INVALID_QUERY_HANDLE, PrintId(query_id)); http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2480 PS20, Line 2480: for (const auto& backend : other_coordinators->GetAllExecutorDescriptors()) { nit: I think it is possible to send the RPC asynchronously to all coordinators at once. Please check if it possible. This, however, is simple enough as initial implementation. See also: krpc-data-stream-sender.cc, KrpcDataStreamSender::Channel::DoTransmitDataRpc, and KrpcDataStreamSender::Channel::TransmitDataCompleteCb. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2494 PS20, Line 2494: const int num_retries = 3; : const int64_t timeout_ms = 10 * MILLIS_PER_SEC; : const int64_t backoff_time_ms = 3 * MILLIS_PER_SEC; Please double check how this will interact with query_timeout_s and fetch_rows_timeout_ms option. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2500 PS20, Line 2500: RETURN_IF_ERROR(rpc_status); Should this move on to the next Coordinator on error? query_id might live inside the next Coordinator instead of the erroring one. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2505 PS20, Line 2505: // Kill succeeded. Please LOG a message if the Coordinator is found (here and L2509). 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: VLOG_QUERY << "Cancel(): query_id=" << PrintId(query_id); Add information whether the request comes from KILL query or client RPC. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/impala-server.cc@1812 PS20, Line 1812: CancelInternal(query_id) Add parameter to say where the cancel request is coming from: KILL query, or client RPC. http://gerrit.cloudera.org:8080/#/c/21930/13/tests/custom_cluster/test_kill_query_custom_cluster.py File tests/custom_cluster/test_kill_query_custom_cluster.py: http://gerrit.cloudera.org:8080/#/c/21930/13/tests/custom_cluster/test_kill_query_custom_cluster.py@76 PS13, Line 76: > Because the HS2 clients used in the test suite do not support authenticatio Ack http://gerrit.cloudera.org:8080/#/c/21930/13/tests/query_test/test_kill_query.py File tests/query_test/test_kill_query.py: http://gerrit.cloudera.org:8080/#/c/21930/13/tests/query_test/test_kill_query.py@74 PS13, Line 74: assert_kill_ok(client, query_id_to_kill) : assert_kill_error( : client, : "Could not find query on any coordinator", : query_id=query_id_to_kill, : ) : assert_kill_error( : client, : "Could not find query on any coordinator", : query_id=query_id_to_kill, : ) : : > These two functions are also used in test_kill_query_custom_cluster.py and Ack http://gerrit.cloudera.org:8080/#/c/21930/13/tests/query_test/test_kill_query.py@89 PS13, Line 89: > Thanks! Added a dimension in test_cancellation.py. Done 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 > Thanks! Moved the utils to this file and removed the two __init__.py files. I think cluster_config.py content should merge into custom_cluster_test_suite.py because it is only relevant for CustomCluster test. 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: # 'use_kill_query_statement' and 'join_before_close' cannot be both True, since : # the KILL QUERY statement will also close the query. : if join_before_close and use_kill_query_statement: : return Write this as a constraint in TestCancellation. # 'use_kill_query_statement' and 'join_before_close' cannot be both True, since # the KILL QUERY statement will also close the query. cls.ImpalaTestMatrix.add_constraint( lambda v: not (v.get_value('use_kill_query_statement') and v.get_value('join_before_close'))) Then, turn this into assert: assert not (join_before_close and use_kill_query_statement) -- 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: 20 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: Thu, 02 Jan 2025 23:30:39 +0000 Gerrit-HasComments: Yes
