accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William Slacum <>
Subject Re: Resource leak warnings
Date Mon, 30 Dec 2013 14:36:15 GMT
At best the javadoc is incomplete and at worst incorrect. If it were just
representing configuration information, it would be a structure containing
only fixed data like the zookeeper list and timeout. Instead, it creates
resources and has a direct handle to those resources via its own ZooCache
property and it contains convenience methods to create other resources like
connectors. A javadoc comment is enough to warrant ignoring resource

Storing state statically one thing, not cleaning up after ourselves is
another. We don't need a whole new API to do that because we've already
done that with the addition of `close()`. Keeping a list of
ZooKeeperInstances to close is already provides the same functionality as
just shutting down everything with the utility, as well as the ability to
free a subset of the resources.

That being said, has anyone started on the utility so we can at least have
a comparison/bake off? I assume this is going to block 1.6.0/1.5.1.

On Fri, Dec 27, 2013 at 6:52 PM, Christopher <> wrote:

> 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
> On Fri, Dec 27, 2013 at 2:23 PM, William Slacum
> <> 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 <
> >wrote:
> >
> >> On Dec 26, 2013 12:27 PM, "Mike Drob" <> 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.
> >>

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message