FYI, I filed ACCUMULO-2128 to track this https://issues.apache.org/jira/browse/ACCUMULO-2128
On Thu, Jan 2, 2014 at 5:04 PM, Sean Busbey <[email protected]>wrote: > Excellent! it sounds like we have consensus. > > Keith, I think you have volunteered for both the revert work and > reimplementing the PoC Hammer. > > Do you want help in either of those cases? Help with a functional test, > docs? > > > On Thu, Jan 2, 2014 at 5:00 PM, William Slacum < > [email protected]> wrote: > >> Voting for the hammer/hacksawjimdugging. I like the concept of being to >> track resources and clean them up, but the back end code isn't designed to >> deal with an instance in the way we're trying to model it. >> >> >> On Thu, Jan 2, 2014 at 2:46 PM, Josh Elser <[email protected]> wrote: >> >> > Bill Slacum and I had talked about unexpected breakages in API for >> clients >> > and internal by modifying ZooKeeperInstance (I think I might have >> mentioned >> > it already on one of the tickets). >> > >> > Considering some of the other work that Mike has started on in regards >> to >> > making an easier-to-use client API, Bill and I mused over an >> > InstanceFactory notion which could wrap different Instance >> implementations >> > for the various deployment requirements. We could leave the current ZKI >> > (close to?) how it works now, lift the non thread-safe pieces to a >> common >> > parent, and create some sort of ThreadsafeZKI. >> > >> > Obviously this is very hand-wavy, but I'm definitely leery to changing >> the >> > default impl for something so prevalent as ZKI. Thinking about the >> problem >> > with a clean slate seems best to me. >> > >> > >> > On 1/2/14, 1:36 PM, Eric Newton wrote: >> > >> >> All of our current code treats the Instance like a simple record: >> >> >> >> * immutable, and therefore >> >> * thread-safe >> >> * provides several fields that describe an instance >> >> >> >> When I tried to add calls to close() in our own code, I found that our >> >> disregard for the lifetime of an instance was implicit, and probably >> is in >> >> all our user's code, too. >> >> >> >> I think if we want to do something like #1, we'll have to do so >> through a >> >> new API, and not by changing Instance, and then deprecate Instance. >> The >> >> mental model is just completely different. >> >> >> >> -Eric >> >> >> >> >> >> On Thu, Jan 2, 2014 at 12:47 PM, Sean Busbey < >> [email protected]> >> >> wrote: >> >> >> >> Hey Folks! >> >>> >> >>> We need to come to some conclusions on what we're going to do for >> >>> resource >> >>> clean up. I'll attempt to summarize the situation and various options. >> >>> If I >> >>> missed something from our myriad of tickets and mailing list threads, >> >>> please bring it up. >> >>> >> >>> Brief Background: >> >>> >> >>> The existing client APIs presume that a large amount of global state >> will >> >>> persist for the duration of a JVM instance. This is at odds with >> >>> lifecycle >> >>> management in application containers, where a JVM is very long lived >> and >> >>> user provided applications are stood up and torn down. We have >> reports of >> >>> this causing OOM on JBoss[1] and leaked threads on Tomcat[2]. >> >>> >> >>> We have two possible solutions, both of which Jared Winick has kindly >> >>> verified solve the problem, as seen on JBoss. >> >>> >> >>> ---- >> >>> = Proposed solution #1: Closeable Instance >> >>> >> >>> The first approach adds a .close method to Instance so that users can >> say >> >>> when they are done with a given instance. Internally, reference >> counting >> >>> determines when we tear down global resources. >> >>> >> >>> Advantages: >> >>> * States via code where a client should do lifecycle management. >> >>> * Allows shutting down just some of the resources used. >> >>> * Is already in the code base. >> >>> >> >>> Disadvantages: >> >>> * Since lifecycle is getting added post-hoc, we are more likely to >> >>> have >> >>> maintenance issues as we find other side effects we hadn't considered, >> >>> like >> >>> the multithreaded issue that already came up[3]. >> >>> * Changes Instance from representing static configuration to shared >> >>> state >> >>> * Doesn't work with the fluent style some of our APIs encourage. >> >>> * closed semantics probably aren't consistently enforced (e.g. >> users >> >>> trying to use a BatchWriter that came from a now-closed instance >> should >> >>> fail) >> >>> >> >>> To finish, we'd need to >> >>> * Verify multithreaded handling is done without too much of a >> >>> performance >> >>> impact[3] >> >>> * Finish making our internal use consistent with the lifecycle >> we're >> >>> telling others to use[4] >> >>> * Possibly add tests to verify consistent enforcement of closing on >> >>> objects derived from Instance >> >>> >> >>> = Proposed solution #2: Global cleanup utility, aka The Hammer >> >>> >> >>> As a band-aid to allow for "unload resources" without making changes >> to >> >>> the >> >>> API we instead provide a utility method that cleans up all global >> >>> resources. >> >>> >> >>> Advantages: >> >>> * Doesn't change API or meaning for Instance >> >>> * Can be used on older Accumulo deployments w/o patch/rebuild cycle >> >>> >> >>> Disadvantages: >> >>> * Only allows all-or-nothing cleanup >> >>> * Doesn't address our underlying lack of lifecycle >> >>> * Requires reverts >> >>> >> >>> To finish, we'd need to >> >>> * revert commits from old solution (I haven't checked how many >> >>> commits, >> >>> but it's 6 tickets :/ ) >> >>> * port code from PoC to main codebase (asf grants, etc) [6] >> >>> * add some kind of test (functional/IT?) >> >>> >> >>> ----- >> >>> >> >>> We need to decide what we're going to provide as a placeholder for >> >>> releases >> >>> already frozen on API (i.e. 1.4, 1.5, 1.6*) as well as longer term. >> >>> >> >>> Personally, my position is that we should use the simplest change to >> >>> handle >> >>> the published versions (solution #2). >> >>> >> >>> Obviously there are outstanding issues with how we deal with global >> state >> >>> and shared resources in the current client APIs. I'd like to see that >> >>> addressed as a part of a more coherent client lifecycle rather than >> >>> struggling to make it work while maintaining the current API. Long >> term, >> >>> I >> >>> think this means handling things in the updated client API Christopher >> >>> has >> >>> mentioned a few times. >> >>> >> >>> How close to consensus are we already? >> >>> >> >>> - Sean >> >>> >> >>> [1]: https://issues.apache.org/jira/browse/ACCUMULO-1379 >> >>> [2]: https://issues.apache.org/jira/browse/ACCUMULO-1697 >> >>> [3]: https://issues.apache.org/jira/browse/ACCUMULO-2027 >> >>> [4]: https://issues.apache.org/jira/browse/ACCUMULO-1923 >> >>> [6]: https://issues.apache.org/jira/browse/ACCUMULO-2113 >> >>> >> >>> *: I think 1.6 should be in this list because we are at feature >> freeze, >> >>> and >> >>> any proper changes to lifecycle management are likely to constitute an >> >>> addition that wouldn't pass that restriction. Mike Drob suggested in >> chat >> >>> that given the current state of 1.6 a more intrusive fix might be >> >>> acceptable. >> >>> >> >>> >> >> >> > >
