Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 4: (17 comments) Thank you very much! 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 specifyin Yes. I think that is possible and makes sense. As you suggested, we can support killing a set of queries whose IDs are specified as a query. I think that would be very useful. I think we can also support specifying multiple query IDs as a list, like in the VALUES statement. For example: ``` KILL QUERY ('123:456', '234:567', '345:678'); ``` What do you think? Thanks! 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: > Shouldn't need WARN_UNUSED_RESULT since Status is already marked [[nodiscar Thanks! Removed. 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::UNKNOWN: > Nit: reorder below UNKNOWN to match Thrift enum. Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/2/be/src/service/client-request-state.cc@2454 PS2, Line 2454: // The current impalad is the coordinator of the query. > There is a good chance the kill query commands will be run on the coordinat Thanks! Added code to try killing the query locally. 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: ::kudu::rp > Nit: fully qualify to match existing code conventions in this file. Thanks! Changed. 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: Respond > Note: unaddressed TODO Thanks! This is what I want to discuss with reviewers. I agree with you so I removed it. 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 Thanks! I changed them to `repeated` so that we can pass multiple query ids and get multiple statuses back with one RPC. The requesting_user is the user running the KILL QUERY statement, and therefore I think there will still be at most one. http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@469 PS2, Line 469: repeated UniqueIdPB query_ids = 1; > Should be required. Thanks! Changed to `repeated` as explained above. http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@474 PS2, Line 474: repeated > Should be required. Thanks! Changed to `repeated` as explained above. 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: UNKNOWN = 9 > Don't modify the values of existing Thrift enums. KILL must be listed last Thanks! Changed. 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 Thanks! I think that makes sense. But currently in KillQueryStmt we will not get notified if an admin privilege is confirmed. We will only get notified when the check fails. How about adding a callback function that gets called each time a privilege check is passed? http://gerrit.cloudera.org:8080/#/c/21930/2/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java@60 PS2, Line 60: setMaskPrivChecks(null) > I am not very sure if we have to call setMaskPrivChecks() here. Thanks! The masked privilege check is used here because - First, we need to check whether the requesting user is an admin. - Second, we don't want to throw any exception here if the requesting user is not an admin. I think it might be easier to add a custom exception handler for a masked privilege check than an unmasked one. What do you think? Thanks! http://gerrit.cloudera.org:8080/#/c/21930/2/tests/authorization/__init__.py File tests/authorization/__init__.py: PS2: 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/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 test_coordinator_unreachable(self): > No need to define get_workload function. Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/21930/2/tests/custom_cluster/test_kill_query_custom_cluster.py@71 PS2, Line 71: self.create_client_for_nth_impalad(0, protocol) as user2_client, \ > Please also add a test where a non-admin user successfully kills its own qu Thanks! The query is submitted by a non-admin user, user1, and the last line of this test case asserts that user1 can kill the query. Is this OK? http://gerrit.cloudera.org:8080/#/c/21930/2/tests/query_test/__init__.py File tests/query_test/__init__.py: PS2: 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/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 add_test_dimensions(cls): > No need to define get_workload function Thanks! Removed. -- 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: 4 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: Xuebin Su <[email protected]> Gerrit-Comment-Date: Thu, 17 Oct 2024 11:28:45 +0000 Gerrit-HasComments: Yes
