accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josh Elser <josh.el...@gmail.com>
Subject Re: [DISCUSS] API changes to provide resource cleanup
Date Thu, 02 Jan 2014 19:46:38 GMT
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 <busbey+lists@cloudera.com>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.
>>
>

Mime
View raw message