Dan Hecht has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. ......................................................................
Patch Set 7: (3 comments) Other than the remaining questions around the released_resources() bool, this change looks fine to me. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/query-state.h File be/src/runtime/query-state.h: Line 83: bool released_resources() const { return released_resources_; } > the idea with explicit resource release (as opposed to implicit, via some d Okay, yes, that's what was discussed. And yes, the sentence below (that you've removed) was part of what led to to these comments. But also the fact that ReleaseResources() call is currently triggered by refcnt going to 0. i.e. it looks like the resources are also tied to the refcnt as well as the control structure. I suppose you plan to move the ReleaseResources() to a different point (like when all instance executions are complete, which may happen before all references go away)? Even so, given that multiple threads will be accessing the QueryState, I don't see how a single bool is sufficient to safely determine whether the resources are accessible by code outside of this class. So, I'm still not sure what the usefulness of this bool is. Either we'll have to make accessing/updating this bool and accessing/tearing-down the resources atomic, or we'll have rules about when during the query lifecycle the resources are valid (but then this seems to be less explicit). maybe we can defer adding the bool until the change that actually makes use of it, so that getting this change committed doesn't get blocked on this discussion? Line 93: /// Illegal to call any other function of this class after this call returns. > see above. Thanks, this sentence was part of what led to the comment above. http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 90: DCHECK(status.ok()) << status.GetDetail(); > actually, Init() always returns ok. i'm removing this dcheck. thanks -- To view, visit http://gerrit.cloudera.org:8080/4418 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
