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 <[email protected]>wrote: > On Dec 26, 2013 12:27 PM, "Mike Drob" <[email protected]> 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. >
