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 mdrob@cloudera.com designates
 209.85.219.43 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAL5zq9a-Vi9d8Z8YXBR0Hy-JiyiQhFTE9eEagV8qUBNXN-AOcw@mail.gmail.com>
References: 
 <CAL5zq9a-Vi9d8Z8YXBR0Hy-JiyiQhFTE9eEagV8qUBNXN-AOcw@mail.gmail.com>
From: Mike Drob <mdrob@cloudera.com>
Date: Fri, 13 Dec 2013 13:02:53 -0800
Message-ID: 
 <CAFAMeYLCG_OR4FkxWFBdN+w1UA8TxZ9J_OZDg2FXmGHZ6S-t-Q@mail.gmail.com>
Subject: Re: Resource leak warnings
To: dev@accumulo.apache.org
Content-Type: multipart/alternative; boundary=089e013a14c44f3c0504ed70cacb

--089e013a14c44f3c0504ed70cacb
Content-Type: text/plain; charset=ISO-8859-1

Comments Inline.


On Fri, Dec 13, 2013 at 12:44 PM, Christopher <ctubbsii@apache.org> wrote:

> What should we do about all these additional resource leak warnings
> added as a result of ACCUMULO-1984? (ACCUMULO-2010)
>
> As I see it, there's a few options:
>
> 0. Revert the previous patch for ACCUMULO-1984
> 1. Ignore them
> 2. Suppress them
> 3. Fix them
> 4. Remove Closeable from the interface, but leave the close method
>
> I don't like the idea of reverting the patch.
>
> "1" is not really an option for me, because they're creating noise
> that's getting in the way of me debugging stuff I'm working on.
>
> If you're using Eclipse, did you know that you can delete warnings?


> Given that by making the interface "Closeable", we're in effect
> recommending that users close it, we should probably follow our own
> recommendation, so "2" is probably not a good idea, and "3" is
> probably better. I don't have time to go back and do "3", though.
>
>
What is your time frame for 3 happening? I obviously can't promise anything
for today, but can attempt to address this next week.


> "4" might be a good option, at least for 1.4.5-SNAPSHOT and
> 1.5.1-SNAPSHOT, so we don't convey the idea (which represents a change
> in API semantics) that you *should* close the Instance. Rather, it
> conveys the idea that it's optional, which I think is more consistent
> with those previous versions, and is suitable for the vast majority of
> use cases.
>

Making things Closeable was an intermediate step to making them
AutoCloseable later. As you said in chat, these should have always been
Closeable, but we just never gave clients a way to do that. It caused tons
of heartache for users running from inside of web containers, and I think
the existence of the interface is a strong hint for best practices.


> All of this is completely overshadowing the real issue, though, which
> is that the close method doesn't actually prevent the resources from
> being opened again. It's a superficial fix, that doesn't really
> enforce it. Our API looks like it's stateless, with factory methods...
> but it's not actually stateless. We can close the instance, but the
> resources that were left open aren't isolated to the instance... they
> are used inside the Connector and below. Closing the instance may free
> up resources, but it doesn't stop new ones from being opened again
> inside the connector and below. The problem is that the "Instance"
> object does not fully represent the resources used inside client code,
> so closing it is semantically unintuitive, incorrect, and functionally
> broken if not used in a very specific way.


Can you elaborate on this? Closing an instance should close all of the
underlying objects as well (and I was under the impression that it did).


> For the time being, I'm going to pursue option "4", so I can proceed
> with working on things I need to work on, without all the noise.
>

Loosely related comments, but probably separate points for discussion:
>
> A. It'd be nice to require that contributions do not introduce
> compiler warnings (or malformed javadocs) before applying them.
>

Sure.


> B. The option to revert is much harder to consider seriously when
> we're simultaneously developing 3 releases, because of the merge
> nightmare: you not only have to revert the patch, but also revert the
> merges, which is not a quick action, because it could result in
> conflicts. Reverting is much more daunting in this scenario. Merge
> windows might help, by providing scheduled times for merging work to a
> common branch, which means that reverts can be considered in a more
> timely manner, because we'll know that new code only shows up during a
> predictable window.
>
> --
> Christopher L Tubbs II
> http://gravatar.com/ctubbsii
>

--089e013a14c44f3c0504ed70cacb--