Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 9: (5 comments) This change is looking good. A few minor comments/questions to address. Also, the new kill query statements will need to be automatically admitted through admission control. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2473 PS8, Line 2473: Status status = > Thanks! I think this is what the current code does. That is, it will contin I missed that line 2480 returns if the status represents an error. Thus, if CancelInternal() returns a general error because the query has not yet started, then that line will kick the error back to the client. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2473 PS8, Line 2473: Status status = > The CancelInternal() function can return a general error if the coordinator Done http://gerrit.cloudera.org:8080/#/c/21930/9/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/9/be/src/service/client-request-state.cc@2466 PS9, Line 2466: Status ClientRequestState::ExecKillQueryRequest() { This function is somewhat difficult to follow because it take a few different actions. Please consider refactoring this function into smaller parts (for example, one function to attempt to kill the query locally and another to attempt to kill it remotely). http://gerrit.cloudera.org:8080/#/c/21930/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/21930/9/be/src/service/impala-server.h@729 PS9, Line 729: friend class ClientRequestState; I'm not seeing that this friend class declaration is needed. http://gerrit.cloudera.org:8080/#/c/21930/9/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/9/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@280 PS9, Line 280: // Do nothing Please expand this comment to explain why this exception is being ignored and when subclasses should override this method. -- 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: 9 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: Fri, 13 Dec 2024 22:33:44 +0000 Gerrit-HasComments: Yes
