Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAMz+DuvUZrg-2FyoL68+PbLqMn0Y4TKrPf2duvCkVCH-2XxRqg@mail.gmail.com>
References: 
 <CAL5zq9a-Vi9d8Z8YXBR0Hy-JiyiQhFTE9eEagV8qUBNXN-AOcw@mail.gmail.com>
	<CABwccFGRfwtrnByug3Qn52iSTZJXqtBSx51KS1M4qZaD6UUnLg@mail.gmail.com>
	<CAL5zq9axWU++o5LS72XRaSO5T3HbkHM8gemdCR2HGkz3AwNJSQ@mail.gmail.com>
	<CABwccFF9wN9je1GS-=m1pxv2OWgx=581iKVsC-+AecsN-Ha_=g@mail.gmail.com>
	<CAL5zq9a46UoOV43_EdJHUmeY-8LKgxDjbnDOWYSvGT80iYiW_Q@mail.gmail.com>
	<CAJRvFdoTDste36BM5S7cJQs=zP8noev3SLMtpS+njKaS07Nkkg@mail.gmail.com>
	<CAL5zq9bLcbcSGF7T+2dTS6igHyokWjd1yMbNV+gxWfQDK53gng@mail.gmail.com>
	<CAD-fFUJ2JCou_NW9rZrj6S5+8tod4c93SPhnj74PUPqSWgKM2A@mail.gmail.com>
	<CAL5zq9ZR5JtQWy0FuRwf2kjBkwpuD5vC6_RwYbusMw6+jO=BYg@mail.gmail.com>
	<CAGUtCHrnC-4q1rcAy8MMS+njWY=byN9M2dQe0U2kpAqKeS5S9g@mail.gmail.com>
	<CAFAMeY+ceRqPW1vxEXjapg6dJB25kca94VE06RD8AkQDE6xm2w@mail.gmail.com>
	<CAGUtCHrThjFW-u+JK+ZYALB-qut2VNd5AH1Sb8o7UcQFUXM4xA@mail.gmail.com>
	<CAFAMeYK_2b+XJuHJt2s=L9gfkjvWFFD3vP=miNQ+yRnuutWSCQ@mail.gmail.com>
	<CABwccFGTMMHi6gaEcKi-7W-_OuKXFF+xY-OCSz4FEhWB6HgDvA@mail.gmail.com>
	<CAMz+DuvUZrg-2FyoL68+PbLqMn0Y4TKrPf2duvCkVCH-2XxRqg@mail.gmail.com>
Date: Fri, 27 Dec 2013 18:52:37 -0500
Message-ID: 
 <CAL5zq9ac7mmE6Q-AEiZ30z_abMftH3Hjaq5vToKStFyAX8njHw@mail.gmail.com>
Subject: Re: Resource leak warnings
From: Christopher <ctubbsii@apache.org>
To: Accumulo Dev List <dev@accumulo.apache.org>
Content-Type: text/plain; charset=UTF-8

The javadoc for Instance says: "This class represents the information
a client needs to know to connect to an instance of accumulo."

There's no mention of connection resources or shared state, or any
indication that it is used for anything other than a one-time method
to get a connection... it seems to be defined as configuration
information. The fact that we're talking about it representing
connection resources (which aren't even stored in ZooKeeperInstance
itself, but happens to use some of the shared state we're talking
about for its own implementation), is a bit confusing in the context
of the declared semantics from the javadoc.

The fact is, we store state statically, as global resources, in the
JVM, and (I think) changing the definition of Instance to represent
this statically stored state, is very confusing. I think a static
utility makes a lot more sense to clean up static shared state hidden
deep in the implementation... until we can invent (in a new API) an
actual ConnectionResources object to represent connection resources,
with a well-defined lifetime (not "for the duration of the JVM's
lifetime", as it currently is defined in released versions) where the
cleanup of these resources makes sense.

--
Christopher L Tubbs II
http://gravatar.com/ctubbsii


On Fri, Dec 27, 2013 at 2:23 PM, William Slacum
<wilhelm.von.cloud@accumulo.net> wrote:
> We need to actually define the usage pattern and lifetime of a
> ZooKeeperInstance. Looking at the code, it's really masking a singleton
> usage pattern. The resources backing a given set of zookeepers+timeout pair
> all share a ZooCache, and we hand-rolled reference counting for
> ZooKeeperInstances themselves. That indicates that a ZooKeeperInstance is
> basically a global variable, and we have to be careful about the resources
> it allocates, directly or indirectly, because their lifetimes are opaque
> from the perspective of the client.
>
> I'm a fan of the close method, because it puts, in code, how an instance
> tidies up after itself. We didn't have any cleanup before because the
> ZooCache for a given zookeeper+timeout lived on until the process died.
> Since the side effects of our API aren't documented or made clear to the
> client, it's on us to handle and manage them. Making it optional for a user
> is a benefit, because maybe they don't care and someone else (gc, another
> management thread) will call close() on the instance, or maybe they want to
> force a close at class unloading.
>
> The utility seems to be brute forcing shutdown- is it possible to get
> something finer grained for specific instances? Shutting down every thing
> will handle the "clean up at unload" time issue, but not necessarily
> anything involving closing down a subset of ZooSessions.
>
>
>
> On Thu, Dec 26, 2013 at 2:48 PM, Sean Busbey <busbey@clouderagovt.com>wrote:
>
>> On Dec 26, 2013 12:27 PM, "Mike Drob" <mdrob@cloudera.com> wrote:
>> >
>> > I'm willing to stipulate that this solves the thread leak from web
>> > containers - I haven't verified it, but I am ever hopeful. Does this
>> > solution imply that we should nix the close() methods just added in the
>> > snapshot branches?
>> >
>> >
>>
>> If we can verify that it solves the leaks for web containers, I would say
>> yes.
>>
>> We can do proper life cycle for persistent state when we provide an updated
>> client API.
>>