Sailesh Mukil 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 10: (4 comments) Just have a few minor comments. http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc@1095 PS10, Line 1095: query_state_ != nullptr) How would query_state_ be NULL if the coordinator hasn't release its reference to it yet? We always create a QueryState for a Coordinator and there's no point of failure between when a Coordinator object is created and a QueryState for that Coordinator object is created. http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/mem-tracker.cc@322 PS10, Line 322: while (tracker != nullptr && !tracker->is_query_mem_tracker_) { : tracker = tracker->parent_; : } What's the thread safety story here? What if a any one of the trackers touched here are unregistered from their parent in the middle of this loop? http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc@96 PS10, Line 96: if (query_mem_tracker_ != nullptr) { : // After this point nothing should be touching this query's MemTracker and all : // tracked memory associated with the query must be released. The whole query : // subtree of MemTrackers can be removed from the global tree and destroyed. : query_mem_tracker_->CloseAndUnregisterFromParent(); : } I'm still not clear why this moved from ReleaseExecResources() to ~QueryState(). Doesn't this mean we're releasing the tracked memory later? http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc@135 PS10, Line 135: AcquireExecResourceRefcount(); // Decremented in QueryExecMgr::StartQueryHelper(). Can we move this to the start of Init() and make L95 a DCHECK? -- 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: 10 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 18:37:02 +0000 Gerrit-HasComments: Yes
