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 management.
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 <[email protected]> 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 > http://gravatar.com/ctubbsii > > > On Fri, Dec 27, 2013 at 2:23 PM, William Slacum > <[email protected]> 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 <[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. > >> >
