Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 27: (6 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@2494 PS20, Line 2494: NetworkAddressPBToString(krpc_addr), get_proxy_status.msg().msg())); : } : KillQueryResponsePB response; > Thanks! Checking. Will reply later. It seems that the two query options only affects how the client interacts with the coordinator: - query_timeout_s: the amount of time a query can stay idle before being cancelled. - fetch_rows_timeout_ms: the amout of time the client wants to wait for fetching the results. Is that correct? Thanks! http://gerrit.cloudera.org:8080/#/c/21930/24/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/24/be/src/service/client-request-state.cc@2513 PS24, Line 2513: } else if (status.code() != TErrorCode::INVALID_QUERY_HANDLE) { : LOG(ERROR) << "KillQuery: Found the coordinator at > This should be LOG(ERROR). Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/24/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/21930/24/be/src/service/impala-server.h@858 PS24, Line 858: /// be recorded in the INFO log. > I'd lean towards defaulting from_kill_query_statement=false, since it's an Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/24/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21930/24/be/src/service/impala-server.cc@1795 PS24, Line 1795: << ", method=" << (from_kill_query_statement ? "kill_query" : "cancel_rpc"); > Maybe something like this? Thanks! Added LOG both here and in ImpalaServer::KillQuery() http://gerrit.cloudera.org:8080/#/c/21930/24/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/24/tests/custom_cluster/test_kill_query_custom_cluster.py@32 PS24, Line 32: > Just TestKillQuery should be fine. Please rename the file to test_kill_quer Thanks! Changed the name. I think test_coordinator_unreachable and test_single_coordinator require different custom clusters: - test_coordinator_unreachable requires a normal cluster with 3 coordinators and then kill the first to make it unreachable. - test_single_coordinator requires a cluster with only 1 coordinator. 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 > To me, all @cluster_config.xxx are extensions of @CustomClusterTestSuite.wi It seems that Python 2 does not support this because of the “unbound methods”: https://stackoverflow.com/a/48103953 I guess pytest.mark.skipif() used in tests/common/skip.py is probably not a classmethod. -- 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: 27 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: Mon, 06 Jan 2025 07:49:20 +0000 Gerrit-HasComments: Yes
