Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Received-SPF: pass (nike.apache.org: domain of busbey@cloudera.com designates
 209.85.128.52 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAGHyZ6Lg9+Exz-hteu0uLpM4J1supezw_+40bf-85fP7WHB_Ng@mail.gmail.com>
References: 
 <CAGHyZ6JsKn6AuTpD5A6FFbgF1KY1fshGTvbe7rkhnVyRA5Sc+w@mail.gmail.com>
 <CADxc9BkqFdGk_UjzM+gym5C=3D4g9r6aMUP6QOCifM_Lz-1Wjg@mail.gmail.com>
 <52C5C21E.4000504@gmail.com>
 <CAMz+DutjAvan_wv-hEGXBHV28Uh36Bynp7=LvxnheNkGV=o1Xg@mail.gmail.com>
 <CAGHyZ6Lg9+Exz-hteu0uLpM4J1supezw_+40bf-85fP7WHB_Ng@mail.gmail.com>
From: Sean Busbey <busbey+lists@cloudera.com>
Date: Thu, 2 Jan 2014 17:19:44 -0600
Message-ID: 
 <CAGHyZ6Krx423wZGvhNWKypLVpchZj0=ErkzNnYuj=XSBTQDTqA@mail.gmail.com>
Subject: Re: [DISCUSS] API changes to provide resource cleanup
To: "dev@accumulo apache. org" <dev@accumulo.apache.org>,
 Keith Turner <keith@deenlo.com>
Content-Type: multipart/alternative; boundary=047d7b6d80aa978c3004ef0508fd

--047d7b6d80aa978c3004ef0508fd
Content-Type: text/plain; charset=UTF-8

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 <busbey+lists@cloudera.com>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 <
> wilhelm.von.cloud@accumulo.net> 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 <josh.elser@gmail.com> 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 <
>> 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.
>> >>>
>> >>>
>> >>
>>
>
>

--047d7b6d80aa978c3004ef0508fd--