Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 24: (10 comments) 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 > Thanks! Changed and added a new custom cluster test case for this. Done http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2500 PS20, Line 2500: request, &response, quer > I think when 'rpc_status' is an error, it means we failed to send the reque How about this: LOG(ERROR) all non-OK rpc_status, but keep the first one. If Coordinator of query_id is not found after the loop end, return that first non-OK rpc_status. It is also good to report how many Coordinators are failed to reach. http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/client-request-state.cc@2505 PS20, Line 2505: status = Status(resp > Thanks! Added. Done 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: VLOG_QUERY << "KillQuery: Found the coordinator at " : << NetworkAddressPBToString(krpc_addr); This should be LOG(ERROR). LOG(ERROR) << "KillQuery: Found the coordinator at " << NetworkAddressPBToString(krpc_addr) << " but failed to kill the query: " << status.GetDetail(); 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 > Thanks! Added. Done http://gerrit.cloudera.org:8080/#/c/21930/20/be/src/service/impala-server.cc@1812 PS20, Line 1812: ("User '$0' is not autho > Thanks! Added. Done 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: << ", from_kill_query_statement=" << from_kill_query_statement; > This seems a little weird, but I agree it's useful to differentiate. I'd al Maybe something like this? VLOG_QUERY << "Cancel(): query_id=" << PrintId(query_id) << " requesting_user=" << requesting_user << " method=" << (from_kill_query_statement ? "kill_query" : "cancel_rpc"); 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: TestKillQueryCustomCluster Just TestKillQuery should be fine. Please rename the file to test_kill_query.py accordingly. Consider annotating the whole class rather than individual test method to save time restarting cluster (see IMPALA-13503). So put test_coordinator_unreachable and test_single_coordinator in 1 class, test_kill_as_admin and test_kill_as_non_admin in another class, and the rest that does not share the same decorator in their own class. 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! I think putting the cluster configs in a separate file can make it To me, all @cluster_config.xxx are extensions of @CustomClusterTestSuite.with_args. Therefore, it is better to at least put them in the same file, so if a contributor edit one, they will immediately aware of the other. For keeping common prefix, the new decorators can be organized in a single class like: class ClusterConfig: enable_authorization = ... admit_one_query_at_a_time = ... admit_no_query = ... See also similar organization in tests/common/skip.py. If we start wrapping @CustomClusterTestSuite.with_args now, I suspect we'll add more in the future. 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_ > Thanks! Changed. Done -- 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: 24 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 18:45:15 +0000 Gerrit-HasComments: Yes
