Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 4: (27 comments) 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'; > Yes. I think that is possible and makes sense. Looking at this again, the Jira description has a good outline of the desired syntax. Please add comments there with an specific questions about the syntax. 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: > Thanks! Removed. Done 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: > Thanks! Changed. Done 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. > Thanks! Added code to try killing the query locally. Done http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2446 PS4, Line 2446: string_view Is a new import needed for this object? http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2446 PS4, Line 2446: optional Missing #import <optional> http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2446 PS4, Line 2446: std:: Coding standard is to not use namespaces in .cc files unless required (due to name collisions across namespaces imported with using namespace). http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2476 PS4, Line 2476: if (exec_request().kill_query_request.__isset.requesting_user) { Can the optional variable requesting_user be reused here? http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2483 PS4, Line 2483: all_coordinators.GetAllExecutorDescriptors()) { May be able to simplify the code by leveraging the GetFilteredExecutorGroup() function from executor-group.h combined with ExecEnv::GetInstance()->krpc_address() http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2527 PS4, Line 2527: return results.back(); Can we return a slightly more descriptive response here, something like "could not find query on any coordinator"? 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 > Thanks! Changed. Done 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 > Thanks! This is what I want to discuss with reviewers. Done 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 { > Thanks! I changed them to `repeated` so that we can pass multiple query ids Done 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 Done http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@469 PS2, Line 469: repeated UniqueIdPB query_ids = 1; > Thanks! Changed to `repeated` as explained above. Done http://gerrit.cloudera.org:8080/#/c/21930/2/common/protobuf/control_service.proto@474 PS2, Line 474: repeated > Should be required. Done 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 > Thanks! Changed. Done 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 > Thanks! I think that makes sense. But currently in KillQueryStmt we will no Ok, I better understand what is happening now. That variable is an indicator of the result of the call, not an indicator that says admin privileges are present. I withdraw my previous comment. Let's leave the code as-is. 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 Done 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 te Done http://gerrit.cloudera.org:8080/#/c/21930/2/tests/authorization/__init__.py@1 PS2, Line 1: > This file appears to have been accidentally committed. Done 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): > Thanks! Removed. Done 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, \ > Thanks! The query is submitted by a non-admin user, user1, and the last lin Ah, I missed that final line. Yes, the test is ok as-is. 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 Done 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 t Done 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. Done 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): > Thanks! Removed. 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: 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: Tue, 22 Oct 2024 23:00:40 +0000 Gerrit-HasComments: Yes
