Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 9: (9 comments) > Patch Set 8: > > (14 comments) Thanks for reviewing! 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 Thanks! I think this is what the current code does. That is, it will continue with sending RPCs only if the error code is INVALID_QUERY_HANDLE, otherwise it will report the error back to the client. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2477 PS8, Line 2477: // In this case, no more RPCs are needed. The following logic is similar to > Nit: unnecessary blank line. Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2481 PS8, Line 2481: query_id, true, &Status::CANCELLED); > Note: https://gerrit.cloudera.org/#/c/21803/ changes the signature of the U Thanks! Will try to resolve the conflicts after https://gerrit.cloudera.org/#/c/21803/ gets merged. 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. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2491 PS8, Line 2491: // the kill request to all other coordinators. > Nit: change to "all other coordinators". Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2492 PS8, Line 2492: UniqueIdPB query_id_pb; > I'm not understanding the need for this results vector. The first time it Thanks! Removed the vector variable. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2493 PS8, Line 2493: TUniqueIdToUniqueIdPB(query_id, &query_id_pb); > Nit: combine these two lines using list initialization: Thanks! Removed the vector variable. http://gerrit.cloudera.org:8080/#/c/21930/8/be/src/service/client-request-state.cc@2507 PS8, Line 2507: ExecEnv::GetInst > Nit: use a shorter name for this variable so this line and the one before i Thanks! Combined the two lines with the `auto` keyword. Is that OK? 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: // Request for "KILL QUERY" statements. > Nit: add a comment explaining this definition. Thanks! Added. -- 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: Mon, 18 Nov 2024 08:01:25 +0000 Gerrit-HasComments: Yes
