accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <ctubb...@apache.org>
Subject Re: Resource leak warnings
Date Mon, 23 Dec 2013 03:28:31 GMT
On Sun, Dec 22, 2013 at 2:23 PM, Bill Havanki <bhavanki@clouderagovt.com> 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

Mime
View raw message