Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAJRvFdoTDste36BM5S7cJQs=zP8noev3SLMtpS+njKaS07Nkkg@mail.gmail.com>
References: 
 <CAL5zq9a-Vi9d8Z8YXBR0Hy-JiyiQhFTE9eEagV8qUBNXN-AOcw@mail.gmail.com>
	<CABwccFGRfwtrnByug3Qn52iSTZJXqtBSx51KS1M4qZaD6UUnLg@mail.gmail.com>
	<CAL5zq9axWU++o5LS72XRaSO5T3HbkHM8gemdCR2HGkz3AwNJSQ@mail.gmail.com>
	<CABwccFF9wN9je1GS-=m1pxv2OWgx=581iKVsC-+AecsN-Ha_=g@mail.gmail.com>
	<CAL5zq9a46UoOV43_EdJHUmeY-8LKgxDjbnDOWYSvGT80iYiW_Q@mail.gmail.com>
	<CAJRvFdoTDste36BM5S7cJQs=zP8noev3SLMtpS+njKaS07Nkkg@mail.gmail.com>
Date: Sat, 21 Dec 2013 17:47:06 -0500
Message-ID: 
 <CAL5zq9bLcbcSGF7T+2dTS6igHyokWjd1yMbNV+gxWfQDK53gng@mail.gmail.com>
Subject: Re: Resource leak warnings
From: Christopher <ctubbsii@apache.org>
To: Accumulo Dev List <dev@accumulo.apache.org>
Content-Type: text/plain; charset=UTF-8

tl;dr - skip to the dotted line "......" if you don't care about the
discussion of procedure and just want to move on to the topic of how
best provide a way to clean up resources in the API.

Mike, et. al.-

Well, I certainly apologize if there were any hurt feelings. It was
not my intention to circumvent consensus or community involvement. I
was just trying to open the conversation, while still proceeding with
a slight improvement to the work already done that allowed progress,
but didn't undo that work already done. Even after receiving feedback,
I didn't see any response that was a blocker for my proceeding with
what I indicated I was going to do, as that feedback was not mutually
exclusive to what I had said I was going to do for an improvement.

As for the temporary git workaround... I can try that in the future...
but it's still a lot of hardship to work around this in 3 different
branches, and then dealing with the side-effects (it's not a free
solution... it comes with trade-offs). I might have been more willing
to pursue this as a temporary solution in one branch if I knew that
fix was imminent. I did notice that some commits have since been made
(which broke the M/R tests) to close some of the instances... but it's
not complete.

What I was going for was a minimal change solution that didn't undo
the previous work, but still left open the possibility of improvement
on the foundation that was already laid (unless we come up with a
better solution). I was not trying to undermine anybody's efforts or
unilaterally make decisions (outside the committer norms, that is). I
was simply trying to make an improvement to the previous commit, and
provide an explanation for transparency and discussion on the mailing
list so we could take further action, if warranted. We make these
sorts of improvements to previous commits/contributions all the time
(sometimes without discussion at all, if the improvements are as minor
as this one was), and it doesn't cause issue, so I don't understand
why it should have generated any controversy or anger this time, but I
nevertheless apologize for the fact that it evidently did.

I think most of this can really be address, though, with some bylaws
that establish standard procedures for bad, or incomplete commits,
introduction of new warnings, backporting, reverting bad patches vs.
making a minor mod to improve them, etc. I think we should make it a
priority in 2014 to put together those guidelines in bylaws.

.........................................................................
Getting back to what to do about resources...

For the present:

Keith and I brainstormed a little bit... and he suggested that there
might be a way to have the RPC threads automatically clean up
themselves, and I suggested that we could provide an Instance
constructor that takes a ZooKeeper object that the user can
manage/close. This solves the cleanup issue without incorporating
confusing API semantics of a close() method... but it depends on the
somebody putting in the work to do the auto-cleanup of RPC threads.
Keith seemed like he knew how that could be implemented, but right
now, it's beyond me.

For the future:

A better solution might be to localize all connection-state resources
such that resources are not shared between connection-configuration
(Instance), a factory (Connector) and the objects it produces
(BatchWriter, etc.). Objects created by the factory can either be
closeable themselves to clean up their own resources, or a shared
connection resource object can be closeable and used by other objects.
I've been thinking a lot about a new, better, cleaner API, based on
lessons learned here and elsewhere... and I want to pursue it as part
of new accumulo-client-api and accumulo-client-runtime modules (JIRA
is down right now, so I can't recall the relevant ticket number(s)).

--
Christopher L Tubbs II
http://gravatar.com/ctubbsii


On Thu, Dec 19, 2013 at 11:43 PM, Mike Drob <mdrob@mdrob.com> wrote:
> This conversation is way past dead at this point, but I wanted to respond
> now that things have had a time to cool off. I didn't really pursue a
> response last week because I felt like you [Christopher] had unilaterally
> imposed your will on the rest of us and were unwilling to discuss things
> further. I was pretty upset and decided to just step back from the
> conversation completely.
>
> For the record, you don't have to keep things rebased, you can do a "git
> update-index --assume-unchanged" on the closeable files in your workspace
> and that would magically solve all your warnings. (I love that git is
> magical).
>
> The larger discussion that we need to have is what we do about the API
> problems, and the long-lived resources. There has been some discussion in
> IRC, on various JIRAs, and sprinkled across email about the proper
> solution, but I'm having a hard time mentally merging all of those
> conversations, so I'll propose that we refocus on it here.
>
> What are our invariants? What are the goals? What tools are available to
> solve the problem?
>
> Mike
>
>
> On Fri, Dec 13, 2013 at 4:03 PM, Christopher <ctubbsii@apache.org> wrote:
>
>> On Fri, Dec 13, 2013 at 4:34 PM, Sean Busbey <busbey+ml@clouderagovt.com>
>> wrote:
>> > Could you just do #4 as a patch on your own local repo? It should be
>> > relatively easy to keep rebased given that it's just an interface.
>> [snip]
>>
>> No, I cannot easily keep 3 branches rebase'd, nor do I wish to
>> constantly rebase and reorder my other work that I wish to push,
>> waiting on this issue. I do not see an issue with this (essentially)
>> partial revert, of what was (in my view) incomplete work. The option
>> that seemed to be favored (by myself, included) was #3. Proceeding
>> with #4 in the meantime does not preclude #3 from proceeding, but it
>> does address the immediate issues at hand. We make improvements on
>> changes all the time. I don't see this as any different.
>>
>> --
>> Christopher L Tubbs II
>> http://gravatar.com/ctubbsii
>>