Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers ......................................................................
Patch Set 2: (4 comments) Maybe I've been staring at this too long, but why do we have ReleaseResources() instead of Close() where it's used? It's confusing to track which ones are meant to be idempotent and which ones aren't. http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/buffered-block-mgr.cc File be/src/runtime/buffered-block-mgr.cc: PS2, Line 568: mem_tracker_->UnregisterFromParent(); I don't think you have to UnregisterFromParent here, since the query-state will unregister its query tracker shortly after this. It'd be nice to have just 1 place where we ever call UnregisterFromParent, i.e. in QueryState::ReleaseResources. Then we can clarify the policy for calling UnregisterFromParent in its fn comment, and DCHECK it's only called for query trackers. http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: PS2, Line 200: May not have been closed yet. why not? http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: PS2, Line 105: /// Deregisters the MemTracker from it's parent. Can be called after Close() and before : /// destruction to prevent other threads from getting a reference to the MemTracker via : /// its parent. I think we can simplify the protocol further: I think this can be be called specifically for the query tracker on QueryState::ReleaseResources. http://gerrit.cloudera.org:8080/#/c/7492/2/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 260: void RuntimeState::ReleaseResources() { This function doesn't appear to be idempotent right now, looks like calling UnregisterPool() twice with the same resource_pool_ is invalid. Is there a consistent story for what release/close methods should be idempotent and what shouldn't be? I don't see any reason this would be called twice, so can you add a DCHECK(!released_resources_) for now. -- To view, visit http://gerrit.cloudera.org:8080/7492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
