Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 46: (7 comments) Thanks for reviewing! http://gerrit.cloudera.org:8080/#/c/21930/45/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/21930/45/fe/src/main/cup/sql-parser.cup@90 PS45, Line 90: // Whether the token is an "unreserved keyword". > Please add a comment that this should be kept in sync with 'ident_or_unrese Thanks! Added. http://gerrit.cloudera.org:8080/#/c/21930/45/fe/src/main/cup/sql-parser.cup@342 PS45, Line 342: KW_ORC, KW_ORDER, KW_OUTER, > nit: Is there a reason to rearrange this? Just clutters the review. In the previous patch sets, KW_QUERY was not added in alphabetical order. http://gerrit.cloudera.org:8080/#/c/21930/45/fe/src/main/cup/sql-parser.cup@3576 PS45, Line 3576: | KW_CLUSTERED : {: RESULT = new PlanHint("clustered"); :} : | ident_or_unreserved:name : {: RESULT = new PlanHint(name); :} : | ident_or_unreserved:name LPAREN ident_list:arg > These are harmless uses of IDENT, but I think we should change them to iden Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/45/tests/custom_cluster/test_kill_query.py File tests/custom_cluster/test_kill_query.py: http://gerrit.cloudera.org:8080/#/c/21930/45/tests/custom_cluster/test_kill_query.py@33 PS45, Line 33: @pytest.mark.execute_serially : def test_coordinator_unreachable(self): : """ : The coordinator of the query to kill is unreachable. : : It is required that each impalad in the cluster is a coordinator. : """ : protocol = 'hs2' : with self.create_client_for_nth_impalad(0, protocol) as client, \ : QueryToKill( : self, : protocol, : check_on_exit=False, : nth_impalad=2) as query_id_to_kill: : coordinator_to_kill = self.cluster.impalads[2] : coordinator_to_kill.kill() : assert_kill_error( : client, : "KillQuery() RPC failed: Network error:", : query_id=query_id_to_kill, : ) : : @pytest.mark.execute_serially : def test_another_coordinator_unreachable(self): : """ : A coordinator other than the one of the query to kill is unreachable. : : It is required that each impalad in the cluster is a coordinator. : """ : protocol = 'hs2' : with self.create_client_for_nth_impalad(0, protocol) as client, \ : QueryToKill(self, protocol, nth_impalad=2) as query_id_to_kill: : coordinator_to_kill = self.cluster.impalads[1] # impalad 1 is between 0 and 2. : coordinator_to_kill.kill() : assert_kill_ok(client, query_id_to_kill) > This tests does not have specific cluster flag to exercise. Can they turned After running this test, one impalad will be unreachable. I think this will probably affect other tests if we turn this into a regular end-to-end test. What do you think? Thanks! http://gerrit.cloudera.org:8080/#/c/21930/45/tests/custom_cluster/test_kill_query.py@70 PS45, Line 70: @cluster_config.single_coordinator : def test_single_coordinator(self): > Why is it important to tests against cluster with only 1 Impalad? Can this This test is added because the previous patch set had a bug in this case (Thank you for finding it!). http://gerrit.cloudera.org:8080/#/c/21930/45/tests/custom_cluster/test_kill_query.py@97 PS45, Line 97: Make sure KILL QUERY statement can be executed when no query will be admitted. > I think this test can merge with test_admit_one_query_at_a_time. Scenario i I think test_admit_no_query() shows that KILL QUERY statements are not subject to admission control in a straightforward way. What do you think? Thanks! http://gerrit.cloudera.org:8080/#/c/21930/45/tests/util/cancel_util.py File tests/util/cancel_util.py: http://gerrit.cloudera.org:8080/#/c/21930/45/tests/util/cancel_util.py@127 PS45, Line 127: client > Can you use different client to run the KILL query? I think we should probably use the client in the parameter because this is why we have this parameter. What do you think? Thanks! At the end of the function, I think we validate that the fetch results error is either "Cancelled" or "Invalid or unknown query handle" . In the former case, the FetchResults() RPC will close the query, while in the latter case, the query is already closed. What do you think? Thanks! -- 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: 46 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Mon, 20 Jan 2025 08:04:44 +0000 Gerrit-HasComments: Yes
