Dan Hecht 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: (7 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. do we really need to explicitly do the LogError() below? If we still rely on that then it seems we should fix status propagation. 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 now mean that it's part of the query hierarchy? though should we even call this function in that case? 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"? 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 ... since some FIS-local resources are also resources used for this query but those should be owned and released solely by the FIS thread. http://gerrit.cloudera.org:8080/#/c/8303/11/be/src/runtime/query-state.h@258 PS11, Line 258: backend same question 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 destructors, to make the lifetimes super clear. shouldn't this case just be handled in the caller of Init() when !status.ok() (which already has to handle releasing the QS ref count, and the thread creation path already handles this case. we could similarly release the resource ref count if we acquired that sooner before Init can fail). 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 this case? -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 26 Oct 2017 23:26:52 +0000 Gerrit-HasComments: Yes
