accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Drob <>
Subject Re: Resource leak warnings
Date Fri, 13 Dec 2013 21:02:53 GMT
Comments Inline.

On Fri, Dec 13, 2013 at 12:44 PM, Christopher <> 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.


> 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

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message