Dan Hecht 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 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10815/2/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/10815/2/be/src/runtime/coordinator-backend-state.h@262 PS2, Line 262: I'll remove that whitespace before committing. 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: ) { > This unfortunately makes the code look a little less readable. Since we req I'm curious why you feel this is less readable. I tend to think that passing a parameter for dependencies is more readable for the following reason: As you know, impala has a history of having bugs due to object lifetime mismatch issues. These issues arose when objects (of differing lifetimes) were linked together via pointers. That creates implicit dependencies of lifetimes that is hard to reason about. This can be avoided by expressing the dependencies of code explicitly by passing function arguments. It's obvious that the caller of these functions must pass a TQueryCtx with sufficient lifetime for this call. We've also been systematically addressing the lifetime issue by simplifying and making consistent the lifetime of control structures. For the backend executor side, that means all query control structures are owned (and have the same lifetime as) the QueryState. For the coordinator side, we haven't gotten as far on the cleanup but we should (and it'd likely be the ClientRequestState lifetime, once we clean that class up). The other problem with creating links between objects is that it makes it easy to be lazy about defining the right abstractions, which e.g. leads to bi-direction calls between objects (i.e spaghetti code). Also, I don't want to copy the TQueryCtx into each BackendState because then it becomes less clear that this is a query-wide structure and the copies per backend are a waste of memory (though maybe you meant copy a pointer to it?). All that said, given the tight relationship between this class and the Coordinator (and the fact that the Coordinator owns this), I'm okay with keeping a back pointer to the coordinator to access the query metadata. I think both patchsets are okay, but I assume you prefer the second. LMK if not. -- 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: 2 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Wed, 27 Jun 2018 04:07:24 +0000 Gerrit-HasComments: Yes
