I think the answer should be #3 and I am willing to find time to implement it next week.
I agree with your reasoning on #1 and #2. I think #0 is a no-go because the behavior before it is incorrect. As far as I'm concerned, these should have always been closable and users should be closing them. I think Christopher made the argument in chat that essentially we are just now enabling users to do what they should have done all along. That new resources can be opened is immaterial to closing an individual instance a user has up. I'd also say it's inaccurate to imply that we're stateless, because we very obviously are not. I think that makes it a bug in how we handle users closing and opening, not in marking things Closeable. Also, you should not be able to use a Connection that is attached to a now-closed Instance. I would also consider such use a bug. On Fri, Dec 13, 2013 at 2: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 > -- Sean
