Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10815 )
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated ...................................................................... Patch Set 6: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@336 PS1, Line 336: ) { > I'm curious why you feel this is less readable. As we spoke, my basic reasoning was that if we have a function signature like so: def Func(param..) then that would signify that 'param' is used to do 'Func'. In this case: bool Cancel(query_ctx) that would signify, use 'query_ctx' to 'Cancel'. However, 'query_ctx' is used for an orthogonal use case within the function (which is important, but is just not doing any real 'cancel' work). There are cases where this pattern is unavoidable, but I feel like we could do without it here. The second patch set looks much better though. Thanks for doing that. -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Jun 2018 02:58:23 +0000 Gerrit-HasComments: Yes
