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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: > > > > > On Sun, Dec 22, 2013 at 10:28 PM, Christopher <[email protected]> > > wrote: > > > > > > > On Sun, Dec 22, 2013 at 2:23 PM, Bill Havanki < > > [email protected] > > > > > > > > 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 > > > > > > > > > >
