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 23:06:54 GMT
Mike-

Just to add to your point about chaining methods in the context of
held resources in the API, the following does not lend itself to be a
very intuitive way to understand where and when connection resources
can be cleaned and objects are no longer valid to use.

instance.getConnector(user, pass).getInstance().getConnector(user,
pass).getInstance().getConnector(user, pass).getInstance()...


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii


On Mon, Dec 23, 2013 at 4:36 PM, Mike Drob <mdrob@cloudera.com> wrote:
> Keith,
>
> I'm not sure I understand the alternative solution that you and Christopher
> discussed. Can you explain it in a bit more detail, please? I have my own
> interpretation of what I _think_ you were proposing, but I'd rather not
> risk putting words in your mouth. Specifically, I'm interested in the
> auto-cleanup aspect of things.
>
> I do think that the numbered changes you propose are good ideas. I also
> agree with Christopher that we need to seriously examine the API that we
> have, because chaining methods four- or five-deep [e.g. new
> Instance().getConnector().tableOperations().createTable()] to do something
> is not a great user experience. Determining the scope of the changes is
> another community conversation.
>
> Mike
>
>
> On Mon, Dec 23, 2013 at 9:17 AM, Keith Turner <keith@deenlo.com> wrote:
>
>> 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
View raw message