I'm unable to get the warnings to appear either, using the same additions to the compiler plugin config and playing around with it a bit. I even specifically added a method that fails to close a ZooReader (one of the classes in question) and it didn't complain about it. I really think the resource leak warnings are an Eclipse exclusive. Even the javac docs [1] don't list it as a lint option.
I do see a bunch of other warnings under -Xlint, though. These should be resolved. If we get the code to a point where no warnings are emitted - by fixing important ones and suppressing unimportant ones - then we don't need to worry about new important ones being forgotten later. I forgot to mention that I opened ACCUMULO-2026 as a ticket for deciding if and where to re-introduce Closeable. Christopher, your point about adhering to the semantics of Closeable is an excellent one and should guide the work. Bill [1]: http://docs.oracle.com/javase/6/docs/technotes/tools/solaris/javac.html On Mon, Dec 23, 2013 at 3:34 AM, William Slacum < [email protected]> wrote: > We're pretty clear on commit-then-review and lazy consensus, so I don't > really have an issue with regards to the commits. > > That said, I still think ignoring the warnings is the best course of > action. I compiled with warnings on from the command line and don't see a > resource leak warning with Java 6. We voted not to use Java 7, so this > shouldn't be an issue until we make that move. > > This is what I did to check if those warnings were present when building > from the command line. If this isn't sufficient, please let me know. > > 1) `git revert 335f693a4045d2c2501e2ed6ece0493734093143` > 2) Added the following to the configuration block for the > maven-compiler-plugin: > > <compilerArgument>-Xlint:all</compilerArgument> > <showWarnings>true</showWarnings> > <showDeprecation>true</showDeprecation> > 3) `mvn clean compile | grep -i leak` > > > > On Sun, Dec 22, 2013 at 10:28 PM, Christopher <[email protected]> wrote: > > > On Sun, Dec 22, 2013 at 2:23 PM, Bill Havanki <[email protected] > > > > wrote: > > [snip] > > > Although there was no intention of circumventing consensus, looking at > > the > > > email exchange, consensus was clearly not reached. > > > > It is my understanding that typically, in CtR, consensus is needed to > > resolve issues after they are committed, where there is > > conflict/objections. Perhaps it was my misunderstanding of the > > responses, but it was my understanding that while there was no > > consensus on the final solution, there was no objection that would > > have prevented the interim action taken. > > > > > The short time span did > > > not give others the chance to work on eliminating the warnings, as they > > > offered, or to instead come around to just dropping Closeable. > > > > True... the timespan was short. My goal, as stated in the original > > email, was to commit first (just like I might commit any improvement > > to the current state of the code), and I intended the email to just be > > an explanation of the reasoning, as it related to the prior commits, > > and a prompt for discussion of further action. The fact that I > > submitted the email chronologically first was a bit arbitrary. I > > accept blame for the confusion of that, and any inciting wording the > > email may have caused... I probably could have prepped things a bit > > better... I have many personal "lessons learned" from this. :) > > > > > Personally, > > > I am ambivalent about it. In any event, -1923 now exists to > > comprehensively > > > tackle the issue, and I eagerly welcome input and help on it. > > > > > > Removing Closeable did not undo all the work done, but it did undo some > > of > > > it. It's OK to call it that. Sometimes undoing is fine. That part of > the > > > commit for -2010 is a minimal change. We all agree Closeable should be > > > there eventually, which is more important. We'll get it back. > > > > "undo" or "improve upon" is probably a semantic difference... but > > yeah, my intent was to make it trivial to re-introduce if we decided > > it was best to keep it. > > > > However, I'm not sure we all agree that Closeable should be there > > eventually. I cannot speak for Keith Turner (hopefully, he'll chime in > > at some point), but he and I have discussed this a bit, and I get the > > distinct impression that he thinks it should not be there. > > > > > I never saw any compiler warnings because I don't use Eclipse. I can > > > appreciate wanting to kill annoying warnings, but it would have been > > better > > > to tell Eclipse to STFU about them, until we could come around to > > resolving > > > them. If and when we do introduce some pertinent bylaws, the > > peculiarities > > > of an IDE should not drive them. Tools are there to help us, not tell > us > > > what to do. > > > > It's my understanding that these aren't Eclipse warnings, these are > > default JDK1.6 compiler warnings. I could be wrong here... they may > > need "javac -Xlint:all", or some other flag, to show up. In any case, > > whether it is Eclipse, or FindBugs, or some other tool reporting > > potential problems, I'm not concerned about them for aesthetics... I'm > > concerned because they hint at potential areas of improvements or > > bugs, that we should inspect with due diligence, and when they become > > numerous, it's hard to actually tell the difference between a non-bug > > warning that we've ignored and an actual bug warning that we've not > > examined yet. > > > > In any case, the point is moot here, because even if it didn't produce > > a warning, the current implementation does not warrant giving > > incorrect information to the API consumer that it can/should be > > closed, in accordance with Closeable's semantics (as in the case of > > the currently broken MapReduce configuration code... See comment on > > ACCUMULO-1923, which affects our code, and any subclasses of the > > Input/OutputFormat). I would even go so far as to say that this > > warning actually reflects an API bug: Instance does not actually > > conform to Closeable's semantics... because it doesn't free resources > > held by Instance... it frees static resources held elsewhere, and that > > becomes obvious when we actually try to close it in accordance with > > the semantics of Closeable, so it shouldn't be marked as such (until > > we write the code to make it conform to those semantics). > > > > > There should be no committer norm of unilaterality. (OK, for the most > > > obviously trivial of changes, but that's it.) Never mind whether this > > case > > > was unilateral: we can agree that a unilateral action has the chance to > > > make others feel less valued and frustrated … even if the action is a > > > beneficial one! Bylaws are a great way to avoid this, by setting ground > > > rules. They can strike a balance, because we also do not want to be > > > paralyzed by excessive multilaterality. > > > > > > This is all part of the maturing of a software project. We need to > focus > > on > > > it. A healthy community around Accumulo is necessary for it to succeed. > > > > > > Thanks for reading! > > > Bill > > [snip] > > > > Granted, yes, absolutely, agreed, and so on :) > > (to be clear, when I say "committer norms", I mean of the CtR type... > > it's unilateral to a point, until an objection from review) > > > > -- > > Christopher L Tubbs II > > http://gravatar.com/ctubbsii > > > -- | - - - | Bill Havanki | Solutions Architect, Cloudera Government Solutions | - - -
