Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Received-SPF: pass (athena.apache.org: local policy includes SPF record at
 spf.trusted-forwarder.org)
MIME-Version: 1.0
In-Reply-To: 
 <CAMz+DusXpTgQY7c7dmCVf+WFh6VUG7q0ujzB3tjpM8To-8MGpA@mail.gmail.com>
References: 
 <CAL5zq9a-Vi9d8Z8YXBR0Hy-JiyiQhFTE9eEagV8qUBNXN-AOcw@mail.gmail.com>
 <CABwccFGRfwtrnByug3Qn52iSTZJXqtBSx51KS1M4qZaD6UUnLg@mail.gmail.com>
 <CAL5zq9axWU++o5LS72XRaSO5T3HbkHM8gemdCR2HGkz3AwNJSQ@mail.gmail.com>
 <CABwccFF9wN9je1GS-=m1pxv2OWgx=581iKVsC-+AecsN-Ha_=g@mail.gmail.com>
 <CAL5zq9a46UoOV43_EdJHUmeY-8LKgxDjbnDOWYSvGT80iYiW_Q@mail.gmail.com>
 <CAJRvFdoTDste36BM5S7cJQs=zP8noev3SLMtpS+njKaS07Nkkg@mail.gmail.com>
 <CAL5zq9bLcbcSGF7T+2dTS6igHyokWjd1yMbNV+gxWfQDK53gng@mail.gmail.com>
 <CAD-fFUJ2JCou_NW9rZrj6S5+8tod4c93SPhnj74PUPqSWgKM2A@mail.gmail.com>
 <CAL5zq9ZR5JtQWy0FuRwf2kjBkwpuD5vC6_RwYbusMw6+jO=BYg@mail.gmail.com>
 <CAMz+DusXpTgQY7c7dmCVf+WFh6VUG7q0ujzB3tjpM8To-8MGpA@mail.gmail.com>
From: Bill Havanki <bhavanki@clouderagovt.com>
Date: Mon, 23 Dec 2013 09:54:09 -0500
Message-ID: 
 <CAD-fFU+8JqgSTOBb+Jp28VA4HHeVWZGXaDKEDnbtghPpxhwdkw@mail.gmail.com>
Subject: Re: Resource leak warnings
To: dev@accumulo.apache.org
Content-Type: multipart/alternative; boundary=001a1134a7fa0d770b04ee34cebd

--001a1134a7fa0d770b04ee34cebd
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

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.htm=
l


On Mon, Dec 23, 2013 at 3:34 AM, William Slacum <
wilhelm.von.cloud@accumulo.net> 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 <ctubbsii@apache.org> wrote=
:
>
> > On Sun, Dec 22, 2013 at 2:23 PM, Bill Havanki <bhavanki@clouderagovt.co=
m
> >
> > wrote:
> > [snip]
> > > Although there was no intention of circumventing consensus, looking a=
t
> > 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 th=
ey
> > > 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 so=
me
> > 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 b=
e
> > > 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 =85 even if the action is=
 a
> > > beneficial one! Bylaws are a great way to avoid this, by setting grou=
nd
> > > 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 succee=
d.
> > >
> > > 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
> >
>


--=20
| - - -
| Bill Havanki
| Solutions Architect, Cloudera Government Solutions
| - - -

--001a1134a7fa0d770b04ee34cebd--