impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers
Date Fri, 04 Aug 2017 00:10:59 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message