Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201602260733.u1Q7XQZG013737@ip-10-146-233-104.ec2.internal>
Date: Fri, 26 Feb 2016 07:33:26 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Michael Ho <kwho@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Skye Wanderman-Milne <skye@cloudera.com>, Huaisi Xu <hxu@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: 
 =?UTF-8?Q?[Impala-CR](cdh5-2.5.0=5F5.7.0)_IMPALA-3085:_Unregister_data_sinks'_MemTrackers_at_their_Close()_functions.=0A?=
In-Reply-To: 
 <gerrit.1456425439000.I3aec82150a933dc2b261beff41f5f4f022501bfb@gerrit.cloudera.org>
References: 
 <gerrit.1456425439000.I3aec82150a933dc2b261beff41f5f4f022501bfb@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.10-rc0

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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-HasComments: Yes