----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17426/#review33526 -----------------------------------------------------------
server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java <https://reviews.apache.org/r/17426/#comment63053> Is there any way we can bail if this code is called by the constructor? server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java <https://reviews.apache.org/r/17426/#comment63032> this field seems unnecessary server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java <https://reviews.apache.org/r/17426/#comment63036> Nice catch! server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java <https://reviews.apache.org/r/17426/#comment63047> It seems like this warning could have occurred spuriously before your change to copy the map. So your change may be a bug fix, unrelated to fixing the "this" issue. This case of tabletReport being null seems like something that should never happen now. So maybe remove the null check and should it ever happen let the NPE occur. server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java <https://reviews.apache.org/r/17426/#comment63037> this is not operating on the snapshot you created, but doing that wouldn't make sense. Maybe do the following. sync(tabletReports){ Tablet other = tabletReports.remove(ext) if(other != tablet) tabletReports.put(ext, other) } Another alternative would be to make the key for the map be extent AND "something unique to tablet". But I think the syncronized remove block would be good. server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java <https://reviews.apache.org/r/17426/#comment63051> I think tabletResources is unused and can be removed along w/ the methods for adding and removing. - kturner On Jan. 31, 2014, 7:28 p.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17426/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2014, 7:28 p.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 > >
