accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Drob <md...@cloudera.com>
Subject Re: Resource leak warnings
Date Thu, 26 Dec 2013 18:26:33 GMT
I'm willing to stipulate that this solves the thread leak from web
containers - I haven't verified it, but I am ever hopeful. Does this
solution imply that we should nix the close() methods just added in the
snapshot branches?


On Mon, Dec 23, 2013 at 3:37 PM, Keith Turner <keith@deenlo.com> wrote:

> We discussed some complex solutions, but I was also thinking of a very
> simple and direct solution to the problem at hand.  Below is a very
> straightforward solution that gives web users a hammer to wack this problem
> with.   Its a very targeted utility.   Would this work?  In JBoss and
> tomcat, can users execute code on undeployment to call this?  It might be
> possible to write this utility for released versions w/ a little
> reflection.
>
>
> package org.apache.accumulo.core.util;
>
> import org.apache.accumulo.core.client.impl.TabletLocatorImpl;
> import org.apache.accumulo.core.client.impl.ThriftTransportPool;
> import org.apache.accumulo.core.zookeeper.ZooSession;
>
> public class ClientThreads {
>   /**
>    * kills all threads created by internal Accumulo singleton resources.
>  After this method is called, no accumulo client will work in the current
> classloader.
>    */
>   public static void shutdownNow(){
>     //kills all threads in transport pool and closes all connections, after
> called no threads or connections can be created
>     ThriftTransportPool.shtudownNow();
>
>     //closes all zookeepers, and does not allow any more to be created
>     ZooSession.shutdownNow();
>   }
> }
>
>
> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message