accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jared Winick <jaredwin...@gmail.com>
Subject Re: Resource leak warnings
Date Tue, 24 Dec 2013 14:47:35 GMT
While not following this thread completely, to address your question about
executing code on undeployment, the answer is yes. For Java web
applications you need to just implement the
javax.servlet.ServletContextListener interface. An example can be seen at
https://github.com/jaredwinick/accumulo-1858-test which is just a trivial
web application used for testing the Instance.close() method when the web
application is undeployed.


On Mon, Dec 23, 2013 at 4: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