accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 17426: ACCUMULO-1948 Tablet constructor no longer leaking this
Date Tue, 04 Feb 2014 16:32:53 GMT


> On Feb. 3, 2014, 6:49 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java, line 302
> > <https://reviews.apache.org/r/17426/diff/4/?file=457845#file457845line302>
> >
> >     Is there any way we can bail if this code is called by the constructor?
> 
> Bill Havanki wrote:
>     We could maybe introduce a latch or barrier that blocks or aborts the calls until
it's cleared. The barrier would have to be maintained outside of Tablet, that is, the code
that constructs the Tablet has to trip the latch. It's not safe for the Tablet constructor
to clear it, even as its last instruction.
>     
>     CountDownLatch startingGun = new CountDownLatch(1);
>     Tablet tablet = new Tablet(startingGun, ...);
>     startingGun.countDown();  // bang
>     
>     I'm assuming this would be as a safety measure. Right now I can't see how updateMemoryUsageStats
would be called before tablet construction completes, but admittedly it's complex.
>     
>     Speaking of this escaping, I noticed as I was walking the call graph for updateMemoryUsageStats
that the MemoryManagementFramework inner class of TabletServerResourceManager starts a couple
of threads in its constructor, which is a no-no [1]. I'll file a follow-up JIRA about that.
>     
>     [1] http://stackoverflow.com/questions/5623285/why-not-to-start-a-thread-in-the-constructor-how-to-terminate
> 
> kturner wrote:
>     Yeah I was just thinking about future code changes, maybe nothing needs to be done.
  I wonder if static analysis tools like findbugs would detect indirect "this" leakage.  
Maybe it would be best in the long term to simplify the tablet constructor by moving stuff
out.

They can help in finding patterns that cause it, like kicking off threads in a constructor,
but we probably also have to stay vigilant ourselves for more subtle cases. It would be a
great thing to add regular tool runs anyway - I like Sonar as a way to manage them, and Jenkins
can do it to a good extent as well.

Simplification yes. ACCUMULO-2041 and ACCUMULO-2255 are already filed for Tablet and TabletServer
refactoring, respectively. There's a lot that could be done there!


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17426/#review33526
-----------------------------------------------------------


On Feb. 4, 2014, 11:27 a.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17426/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2014, 11:27 a.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-1948
>     https://issues.apache.org/jira/browse/ACCUMULO-1948
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> Change in the construction of the cross-referencing between Tablet and TabletResourceManager
to avoid the Tablet constructor leaking this.
> 
> 
> Diffs
> -----
> 
>   server/tserver/pom.xml b627de092faa2b5b4dbc5f8b9e4fb7af696ca436 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java 5ea2401acc720c70d58ee04c2dd82dc79fcf2b99

>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 9d08c81f9b07a8c5f8a281769fd2914f5c8fc82b

>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
e95843775c85ecf90a2b4c7afce91094381da450 
>   server/tserver/src/test/java/org/apache/accumulo/tserver/TabletResourceManagerTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17426/diff/
> 
> 
> Testing
> -------
> 
> Added unit test. Tested on pseudo-cluster with shell activity, randomwalk MultiTable
test.
> 
> 
> Thanks,
> 
> Bill Havanki
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message