Dan Hecht has posted comments on this change. Change subject: IMPALA-3085: Unregister data sinks' MemTrackers at their Close() functions. ......................................................................
Patch Set 3: Code-Review+2 (1 comment) > (2 comments) > > Yes, I had the same question. It seems we only set auto_unregister_ > to true for the query wide MemTracker and request pool MemTracker > but not for other MemTrackers. My impression is that it was > expecting the callers to clean up properly although we should > revisit whether it makes sense to have it as the default behavior > after this release. We should only do memory management in the destructor (i.e. free memory owned by this object). In other words, let's restrict destructors to be our "garbage collector" but any control logic should be done at an explicit point during the query lifecycle (such as Close()). So, I don't think we should increase usage of "auto_unregister" (instead we should get rid of it). http://gerrit.cloudera.org:8080/#/c/2314/3/be/src/exec/hdfs-sequence-table-writer.cc File be/src/exec/hdfs-sequence-table-writer.cc: Line 310: mem_pool_->FreeAll(); hmm looks like the mem_pool_ itself is never deleted also (other writers have this as a scoped_ptr). -- To view, visit http://gerrit.cloudera.org:8080/2314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3aec82150a933dc2b261beff41f5f4f022501bfb Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
