You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2025/01/02 23:30:39 UTC
[Impala-ASF-CR] IMPALA-12648: Add KILL QUERY statement
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 <xs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jan 2025 23:30:39 +0000
Gerrit-HasComments: Yes