Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8303 )
Change subject: IMPALA-1575: Part 1: eagerly release query exec resources ...................................................................... Patch Set 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/mem-tracker.cc@328 PS11, Line 328: RuntimeState* state > definitely not this change, but should we just get rid of this parameter. d I think the status propagation is a lot more reliable that it used to be. One potential advantage of logging it is if we hit the memory limit in multiple places around the same time we would get more diagnostics in the log instead of whichever error won the relate. Unclear if that's useful or important. We could also consider making the memtracker dump in the status more concise - it's pretty verbose now. http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/mem-tracker.cc@359 PS11, Line 359: was not available > does that comment still make sense? isn't the query_tracker==nullptr case n We do call it on the process MemTracker in QueryState::Init(). http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.h@205 PS11, Line 205: backend > what do we mean by "backend" here? is that the same thing as "execution"? Yeah I don't think that word added anything. Removed. http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.h@206 PS11, Line 206: Resources for this query > How about: Query wide resources ... Done http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.h@258 PS11, Line 258: backend > same question Done http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.cc@95 PS11, Line 95: if (!released_exec_resources_) ReleaseExecResources(); > we've been trying to move away from doing resource releasing from destructo My initial concern was that handling it in Init() would get messy in the case when the coordinator was already holding a resource refcount. I came up with a reasonable solution - acquiring the refcount always and then releasing it on the error path. http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.cc@122 PS11, Line 122: TExecQueryFInstancesParams& non_const_params = I needed to remove this declaration because of the goto crossing the declaration. http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/runtime-state.cc@88 PS11, Line 88: local_query_state_->AcquireExecResourceRefcount(); // Decremented in ReleaseResources(). > why is that needed? what resource needs to be held by the runtime state in The resources used for evaluating exprs, etc - this is used in various test environments and in fe-support. We could also expose QueryState::ReleaseExecResources() and call it directly, but it seemed better to use the same API here as in the main query execution flow. -- To view, visit http://gerrit.cloudera.org:8080/8303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41ff374b0403f10a145f7fee9b3145953ee32341 Gerrit-Change-Number: 8303 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 27 Oct 2017 04:39:07 +0000 Gerrit-HasComments: Yes
