accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <>
Subject Resource leak warnings
Date Fri, 13 Dec 2013 20:44:18 GMT
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.

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.

"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.

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.

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

View raw message