accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Keith Turner <ke...@deenlo.com>
Subject Re: Resource leak warnings
Date Mon, 23 Dec 2013 17:17:54 GMT
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.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
>

Its not just Closeable, I am uncertain about adding a Instance.close() to
the API.  Its a very broad change to solve a very specific problem, which
is not a problem in itself.  My specific concern is that its a big change
to the API w/o much vetting.  I think the following changes should be made
before release.

 1) Modify existing examples to use the new instance.close() call.
 2) Modify existing test to use the new instance.close() call.
 3) Address the warnings
 4) Create test to verify the expected behavior of the new instance.close()
call.  For example verify that Scanners, BatchScanner, tablet operations,
etc all stop working when an instance is closed (if this is the intended
behavior?).  Verify that closing one instance object does not impact other
instance objects.

The purpose of #1, #2, and #3 is to eat our own dog food.  Doing #1, #2,
and #3 would determine what it will be like for users.  This change will
impact all users, its not a dark corner of the API its at the front door.
 If we release with Instance.close(), we will be stuck supporting it for
years to come.  I think it would be good to try to understand the
implications of that the best we can now.   #4 is needed to define the
expected behavior of the new API and ensure its achievable.   Also, I
opened a bug about the new method not achieving its original goal in the
case of multiple threads.

As Christopher said, we have discussed alternative means of solving the web
container problem that are more targeted.  I would like prototype
something, but I have not had time so far.   Don't let my airing of
concerns give the impression that I do not want to see a solution offered
to users.  If there is no alternative solution I have no desire to put up
road blocks for the existing solution.   This is an annoying problem for
which users have no workaround and I really want to give users a way to
address it.



> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message