Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 6: (2 comments) Hi Xuebin, I took another look at the patch and had 2 more findings. Let me know if I missed something important. Thanks! http://gerrit.cloudera.org:8080/#/c/21930/6/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/21930/6/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@185 PS6, Line 185: setRetainAudits(false) I missed this in my previous review. I have the following 2 new comments. - Setting 'retainAudits_' to false would prevent Impala's frontend from generating the corresponding Ranger audit event for the KILL QUERY statement. Do we want to skip the audit logging in this statement? Specifically, when 'retainAudits_' is false, 'tmpAuditHandler' in authorizeAll() of https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java will be null. It looks like Impala's frontend produces a Ranger audit event only if 'tmpAuditHandler' is not null as shown in the following. But since currently for a masked privilege request, we set 'retainAudits_' to false, no Ranger audit will be produced. if (originalAuditHandler != null && tmpAuditHandler != null) { updateAuditEvents(tmpAuditHandler, originalAuditHandler, false /*not any*/, resource.getKeys().contains(RangerImpalaResourceBuilder.STORAGE_TYPE) ? Privilege.RWSTORAGE : privilege); } - The above also reminds me of the fact that for this query, unless the requesting user has the ALL privilege on SERVER, we won't know during the authorization phase (in frontend) whether the requesting user will be able to kill the specified query until the query has been sent to the backend for execution. If that's the case, then how to generate an accurate Ranger audit event that accurately reflects whether or not the requesting user is able to execute the KILL QUERY statement? It's okay we don't resolve this in this JIRA. We could create a follow-up one for this issue. 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@30 PS2, Line 30: SELECT sleep(1000) I have a question about how to manually test the KILL QUERY statement. Specifically, for a query like "SELECT *, SLEEP(10000) FROM functional.alltypes", what should the requesting user see when its query has been killed? I performed the following steps. - Open a terminal to log in to Impala server via impala-shell as the user '$USER', execute the following query. SELECT *, SLEEP(10000) FROM functional.alltypes; - Open another terminal to log in to Impala server via impala-shell as the user 'admin', execute the following query. KILL QUERY 'e2491c5b371c374a:1647bb0300000000'; I could see in admin's terminal that the query has been killed but in the terminal of the user '$USER', there was no notification that the query had been killed. In fact, it looked like $USER's terminal was stuck. Is this expected? Query: KILL QUERY 'e2491c5b371c374a:1647bb0300000000' Query submitted at: 2024-10-30 22:43:32 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=ab44f5b815cdd8f8:828b20bd00000000 +----------------------------------------------------+ | result | +----------------------------------------------------+ | Query e2491c5b371c374a:1647bb0300000000 is killed. | +----------------------------------------------------+ -- 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: 6 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, 31 Oct 2024 05:56:39 +0000 Gerrit-HasComments: Yes
