Voting for #1.
On Fri, Dec 13, 2013 at 3:44 PM, Christopher <ctubb...@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. > > 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 > http://gravatar.com/ctubbsii >
