Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 16: (12 comments) Thanks for reviewing! 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'; > Mention in commit message that we follow syntax from HIVE-17483. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/21930/13//COMMIT_MSG@27 PS13, Line 27: query we want to kill, we need to broadcast the kill request to all the > typo: receiving Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/13/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/21930/13/be/src/service/control-service.cc@282 PS13, Line 282: > This is essentially like issuing CancelOperation then CloseOperation from t Thanks! http://gerrit.cloudera.org:8080/#/c/21930/13/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/21930/13/be/src/service/impala-server.h@731 PS13, Line 731: > nit: instead of this, I think we could provide an ImpalaServer::KillQuery i Thanks! Added ImpalaServer::KillQuery(). http://gerrit.cloudera.org:8080/#/c/21930/13/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/21930/13/common/protobuf/control_service.proto@470 PS13, Line 470: > That would make me feel better about the logic in ExecKillQuery. Thanks! Added 'is_admin'. http://gerrit.cloudera.org:8080/#/c/21930/13/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java File fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java: http://gerrit.cloudera.org:8080/#/c/21930/13/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java@56 PS13, Line 56: if (authzConfig.isEnabled()) { > If authz is not enabled, effectively all users are admins? I think this is the current behavior. Is this OK? http://gerrit.cloudera.org:8080/#/c/21930/13/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/21930/13/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@283 PS13, Line 283: privilege > Nit: privilege Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/13/tests/authorization/__init__.py File tests/authorization/__init__.py: PS13: > Why was this file added? This file is added so that we can import authorization/test_ranger.py in test_kill_query_custom_cluster.py. 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: # ImpylaHS2Connectio > Why not test both protocol everywhere using cls.ImpalaTestMatrix.add_dimens Because the HS2 clients used in the test suite do not support authentication yet. Added comments. http://gerrit.cloudera.org:8080/#/c/21930/13/tests/query_test/__init__.py File tests/query_test/__init__.py: PS13: > Also seems unnecessary. This file is added so that we can import query_test/test_kill_query.py in test_kill_query_custom_cluster.py. 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: : def assert_kill_ok(client, query_id, user=None): : sql = "KILL QUERY '{0}'".format(query_id) : result = client.execute(sql, user=user) : assert result.success and len(result.data) == 1 : assert result.data[0] == "Query {0} is killed.".format(query_id) : : : def assert_kill_error(client, error_msg, query_id=None, sql=None, user=None): : if sql is None: : sql = "KILL QUERY '{0}'".format(query_id) : try: : client.execute(sql, user=user) > Move this under TestKillQuery. These two functions are also used in test_kill_query_custom_cluster.py and cancel_util.py. Removed the `test_suite` argument so that the two functions don't depend on any test suite. http://gerrit.cloudera.org:8080/#/c/21930/13/tests/query_test/test_kill_query.py@89 PS13, Line 89: assert error_msg_expected(str(exc > I like this suggestion. It's worth trying. Thanks! Added a dimension in test_cancellation.py. -- 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: 16 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, 20 Dec 2024 04:17:39 +0000 Gerrit-HasComments: Yes
