Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 2: (16 comments) Good start! http://gerrit.cloudera.org:8080/#/c/21930/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21930/2//COMMIT_MSG@15 PS2, Line 15: KILL QUERY '123:456'; Will it be possible to specify multiple query ids to kill or even specifying a subquery to build the list of query ids to kill? For example: kill query where query_id in (select query_id from sys.impala_query_live where query_State='RUNNING') http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/client-request-state.h@960 PS2, Line 960: WARN_UNUSED_RESULT Shouldn't need WARN_UNUSED_RESULT since Status is already marked [[nodiscard]] http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/client-request-state.cc@336 PS2, Line 336: case TStmtType::KILL: Nit: reorder below UNKNOWN to match Thrift enum. http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/client-request-state.cc@2454 PS2, Line 2454: ExecutorGroup all_coordinators = There is a good chance the kill query commands will be run on the coordinator that owns the query. The code should try killing the query locally first before moving on to the RPCs to the other coordinators. http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/control-service.h File be/src/service/control-service.h: http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/control-service.h@81 PS2, Line 81: RpcContext Nit: fully qualify to match existing code conventions in this file. http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/control-service.cc@276 PS2, Line 276: // TODO: Note: unaddressed TODO FWIW, I favor not waiting for FinishUnregisterQuery() since I do not see any benefits to the end user of waiting additional time for that processing to run. http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@467 PS2, Line 467: message KillQueryRequestPB { Please consider making this object more future proof by modifying the data structure to enable multiple queries owned by multiple users to be killed with one RPC. The KillQueryResponsePB will need modifying too. http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@469 PS2, Line 469: optional UniqueIdPB query_id = 1; Should be required. http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@474 PS2, Line 474: optional Should be required. http://gerrit.cloudera.org:8080/#/c/21930/2/common/thrift/Types.thrift File common/thrift/Types.thrift: http://gerrit.cloudera.org:8080/#/c/21930/2/common/thrift/Types.thrift@113 PS2, Line 113: KILL = 9 Don't modify the values of existing Thrift enums. KILL must be listed last in the enum and be equal to 10. http://gerrit.cloudera.org:8080/#/c/21930/2/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/2/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java@34 PS2, Line 34: requestedByAdmin_ = true Defaulting to true makes me nervous. A better security posture would be to default to false and explicitly set to true after admin privileges are confirmed. http://gerrit.cloudera.org:8080/#/c/21930/2/tests/authorization/__init__.py File tests/authorization/__init__.py: http://gerrit.cloudera.org:8080/#/c/21930/2/tests/authorization/__init__.py@1 PS2, Line 1: This file appears to have been accidentally committed. http://gerrit.cloudera.org:8080/#/c/21930/2/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/2/tests/custom_cluster/test_kill_query_custom_cluster.py@39 PS2, Line 39: def get_workload(cls): No need to define get_workload function. http://gerrit.cloudera.org:8080/#/c/21930/2/tests/custom_cluster/test_kill_query_custom_cluster.py@71 PS2, Line 71: def test_kill_as_non_admin(self): Please also add a test where a non-admin user successfully kills its own query. http://gerrit.cloudera.org:8080/#/c/21930/2/tests/query_test/__init__.py File tests/query_test/__init__.py: http://gerrit.cloudera.org:8080/#/c/21930/2/tests/query_test/__init__.py@1 PS2, Line 1: File appears to have been committed by accident. http://gerrit.cloudera.org:8080/#/c/21930/2/tests/query_test/test_kill_query.py File tests/query_test/test_kill_query.py: http://gerrit.cloudera.org:8080/#/c/21930/2/tests/query_test/test_kill_query.py@91 PS2, Line 91: def get_workload(cls): No need to define get_workload function -- 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: 2 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Wed, 16 Oct 2024 21:41:54 +0000 Gerrit-HasComments: Yes
