Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 8: (14 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'; > Thanks! Added a comment at https://issues.apache.org/jira/browse/IMPALA-126 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: > Thanks! Added. Done http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2476 PS4, Line 2476: // The current impalad is the coordinator of the query. > Thanks! Changed. Done http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2483 PS4, Line 2483: if (status.ok() || status.code() == TErrorCode::INVALID_QUERY_HANDLE) { > May be able to simplify the code by leveraging the GetFilteredExecutorGroup Done http://gerrit.cloudera.org:8080/#/c/21930/4/be/src/service/client-request-state.cc@2527 PS4, Line 2527: RETURN_IF_ERROR(rpc_status); > Can we return a slightly more descriptive response here, something like "co Done 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 = The CancelInternal() function can return a general error if the coordinator owns the query, but the query has not yet started: https://github.com/apache/impala/blob/1f35747ea3677bd004f8ed7b1cc94d6ef2f70c8e/be/src/service/impala-server.cc#L1789-L1792. In that case we do not want to continue with sending out RPCs but rather send back an informative error message stating why the query cannot be cancelled. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2477 PS8, Line 2477: // Nit: unnecessary blank line. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2481 PS8, Line 2481: status = ExecEnv::GetInstance()->impala_server()->UnregisterQuery( Note: https://gerrit.cloudera.org/#/c/21803/ changes the signature of the UnregisterQuery() function by removing the check_inflight parameter. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2489 PS8, Line 2489: } Nit: add blank line after this closing curly brace. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2491 PS8, Line 2491: // the kill request to all coordinators. Nit: change to "all other coordinators". http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2492 PS8, Line 2492: vector<Status> results; I'm not understanding the need for this results vector. The first time it is used the results of the RPC is emplaced at the back. Then, only this one result is referenced. Why not just use the response.statuses(0) directly? http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2493 PS8, Line 2493: results.push_back(status); Nit: combine these two lines using list initialization: vector<Status> results = {status}; http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2507 PS8, Line 2507: other_coordinators Nit: use a shorter name for this variable so this line and the one before it can be combines into a single line. http://gerrit.cloudera.org:8080/#/c/21930/8/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/21930/8/common/thrift/Frontend.thrift@729 PS8, Line 729: 26: optional TKillQueryReq kill_query_request Nit: add a comment explaining this definition. -- 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: 8 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, 14 Nov 2024 20:33:47 +0000 Gerrit-HasComments: Yes
