Updated Branches: refs/heads/master b05d459b9 -> eade3b59d
ACCUMULO-1948 Tablet constructor no longer leaks this The core Tablet constructor no longer sets this on the TabletResourceManager passed to it. The TabletResourceManager instance is created now without a Tablet reference, but only with its key extent and configuration, and so it can be constructed first and safely passed to the Tablet constructor. This alteration in TabletResourceManager drove changes to several other locations which had been expecting a tablet reference to be available. In most of those places, the key extent takes its place. However, Keith Turner pointed out that, without a tablet reference in the TabletStateImpl class (memory report) used to guide minor compaction, it's possible that a report logged against a tablet that was since unloaded could cause compaction to start on a new instance of that tablet that had just been reloaded. In order to ensure that cannot happen, the tablet reference remains, so that the same object is used to decide on compaction and potentially perform compaction. In addition, the method of working with the memory reports was changed so that the same snapshot of them used to decide on compactions is used to find the tablet to compact later. This change was necessary regardless of this ticket. Finally, the unused tabletResources field was removed from TabletServerResourceManager, and that class also now avoids erroneously removing the memory report for a reloaded tablet if its prior incarnation is unloaded in the scenario described above. Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/eade3b59 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/eade3b59 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/eade3b59 Branch: refs/heads/master Commit: eade3b59ddd95732cbfc635eadf6447c6b3108d2 Parents: b05d459 Author: Bill Havanki <bhava...@cloudera.com> Authored: Mon Jan 27 17:09:39 2014 -0500 Committer: Bill Havanki <bhava...@cloudera.com> Committed: Tue Feb 4 14:48:14 2014 -0500 ---------------------------------------------------------------------- server/tserver/pom.xml | 5 ++ .../org/apache/accumulo/tserver/Tablet.java | 9 +-- .../apache/accumulo/tserver/TabletServer.java | 8 +- .../tserver/TabletServerResourceManager.java | 83 ++++++++++---------- .../tserver/TabletResourceManagerTest.java | 49 ++++++++++++ 5 files changed, 105 insertions(+), 49 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/eade3b59/server/tserver/pom.xml ---------------------------------------------------------------------- diff --git a/server/tserver/pom.xml b/server/tserver/pom.xml index b627de0..059473f 100644 --- a/server/tserver/pom.xml +++ b/server/tserver/pom.xml @@ -93,6 +93,11 @@ <scope>test</scope> </dependency> <dependency> + <groupId>org.easymock</groupId> + <artifactId>easymock</artifactId> + <scope>test</scope> + </dependency> + <dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> <scope>test</scope> http://git-wip-us.apache.org/repos/asf/accumulo/blob/eade3b59/server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java ---------------------------------------------------------------------- diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java index 7ce270b..27412db 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java @@ -299,7 +299,7 @@ public class Tablet { commitSession = new CommitSession(nextSeq, memTable); nextSeq += 2; - tabletResources.updateMemoryUsageStats(memTable.estimatedSizeInBytes(), otherMemTable.estimatedSizeInBytes()); + tabletResources.updateMemoryUsageStats(Tablet.this, memTable.estimatedSizeInBytes(), otherMemTable.estimatedSizeInBytes()); return oldCommitSession; } @@ -335,7 +335,7 @@ public class Tablet { deletingMemTable = null; - tabletResources.updateMemoryUsageStats(memTable.estimatedSizeInBytes(), 0); + tabletResources.updateMemoryUsageStats(Tablet.this, memTable.estimatedSizeInBytes(), 0); } } } @@ -365,7 +365,7 @@ public class Tablet { else if (deletingMemTable != null) other = deletingMemTable.estimatedSizeInBytes(); - tabletResources.updateMemoryUsageStats(memTable.estimatedSizeInBytes(), other); + tabletResources.updateMemoryUsageStats(Tablet.this, memTable.estimatedSizeInBytes(), other); } List<MemoryIterator> getIterators() { @@ -405,7 +405,7 @@ public class Tablet { private Configuration conf; private VolumeManager fs; - private TableConfiguration acuTableConf; + private final TableConfiguration acuTableConf; private volatile boolean tableDirChecked = false; @@ -1356,7 +1356,6 @@ public class Tablet { // Force a load of any per-table properties configObserver.propertiesChanged(); - tabletResources.setTablet(this, acuTableConf); if (!logEntries.isEmpty()) { log.info("Starting Write-Ahead Log recovery for " + this.extent); final long[] count = new long[2]; http://git-wip-us.apache.org/repos/asf/accumulo/blob/eade3b59/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java ---------------------------------------------------------------------- diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java index 080cc20..457bd8a 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java @@ -2670,10 +2670,12 @@ public class TabletServer extends AbstractMetricsImpl implements org.apache.accu Tablet[] newTablets = new Tablet[2]; Entry<KeyExtent,SplitInfo> first = tabletInfo.firstEntry(); - newTablets[0] = new Tablet(first.getKey(), TabletServer.this, resourceManager.createTabletResourceManager(), first.getValue()); + TabletResourceManager newTrm0 = resourceManager.createTabletResourceManager(first.getKey(), getTableConfiguration(first.getKey())); + newTablets[0] = new Tablet(first.getKey(), TabletServer.this, newTrm0, first.getValue()); Entry<KeyExtent,SplitInfo> last = tabletInfo.lastEntry(); - newTablets[1] = new Tablet(last.getKey(), TabletServer.this, resourceManager.createTabletResourceManager(), last.getValue()); + TabletResourceManager newTrm1 = resourceManager.createTabletResourceManager(last.getKey(), getTableConfiguration(last.getKey())); + newTablets[1] = new Tablet(last.getKey(), TabletServer.this, newTrm1, last.getValue()); // roll tablet stats over into tablet server's statsKeeper object as // historical data @@ -2895,7 +2897,7 @@ public class TabletServer extends AbstractMetricsImpl implements org.apache.accu boolean successful = false; try { - TabletResourceManager trm = resourceManager.createTabletResourceManager(); + TabletResourceManager trm = resourceManager.createTabletResourceManager(extent, getTableConfiguration(extent)); // this opens the tablet file and fills in the endKey in the // extent http://git-wip-us.apache.org/repos/asf/accumulo/blob/eade3b59/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java ---------------------------------------------------------------------- diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java index e958437..db046e9 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java @@ -16,11 +16,12 @@ */ package org.apache.accumulo.tserver; +import static com.google.common.base.Preconditions.checkNotNull; + import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Map.Entry; import java.util.SortedMap; @@ -82,8 +83,6 @@ public class TabletServerResourceManager { private ExecutorService defaultReadAheadThreadPool; private Map<String,ExecutorService> threadPools = new TreeMap<String,ExecutorService>(); - private HashSet<TabletResourceManager> tabletResources; - private final VolumeManager fs; private FileManager fileManager; @@ -199,8 +198,6 @@ public class TabletServerResourceManager { readAheadThreadPool = createEs(Property.TSERV_READ_AHEAD_MAXCONCURRENT, "tablet read ahead"); defaultReadAheadThreadPool = createEs(Property.TSERV_METADATA_READ_AHEAD_MAXCONCURRENT, "metadata tablets read ahead"); - tabletResources = new HashSet<TabletResourceManager>(); - int maxOpenFiles = acuConf.getCount(Property.TSERV_SCAN_MAX_OPENFILES); fileManager = new FileManager(conf, fs, maxOpenFiles, _dCache, _iCache); @@ -330,12 +327,13 @@ public class TabletServerResourceManager { while (true) { MemoryManagementActions mma = null; + Map<KeyExtent,TabletStateImpl> tabletReportsCopy = null; try { - ArrayList<TabletState> tablets; synchronized (tabletReports) { - tablets = new ArrayList<TabletState>(tabletReports.values()); + tabletReportsCopy = new HashMap<KeyExtent,TabletStateImpl>(tabletReports); } - mma = memoryManager.getMemoryManagementActions(tablets); + ArrayList<TabletState> tabletStates = new ArrayList<TabletState>(tabletReportsCopy.values()); + mma = memoryManager.getMemoryManagementActions(tabletStates); } catch (Throwable t) { log.error("Memory manager failed " + t.getMessage(), t); @@ -344,16 +342,27 @@ public class TabletServerResourceManager { try { if (mma != null && mma.tabletsToMinorCompact != null && mma.tabletsToMinorCompact.size() > 0) { for (KeyExtent keyExtent : mma.tabletsToMinorCompact) { - TabletStateImpl tabletReport = tabletReports.get(keyExtent); + TabletStateImpl tabletReport = tabletReportsCopy.get(keyExtent); if (tabletReport == null) { - log.warn("Memory manager asked to compact nonexistant tablet " + keyExtent); + log.warn("Memory manager asked to compact nonexistent tablet " + keyExtent + "; manager implementation might be misbehaving"); continue; } - - if (!tabletReport.getTablet().initiateMinorCompaction(MinorCompactionReason.SYSTEM)) { - if (tabletReport.getTablet().isClosed()) { - tabletReports.remove(tabletReport.getExtent()); + Tablet tablet = tabletReport.getTablet(); + if (!tablet.initiateMinorCompaction(MinorCompactionReason.SYSTEM)) { + if (tablet.isClosed()) { + // attempt to remove it from the current reports if still there + synchronized(tabletReports) { + TabletStateImpl latestReport = tabletReports.remove(keyExtent); + if (latestReport != null) { + if (latestReport.getTablet() != tablet) { + // different tablet instance => put it back + tabletReports.put(keyExtent, latestReport); + } else { + log.debug("Cleaned up report for closed tablet " + keyExtent); + } + } + } log.debug("Ignoring memory manager recommendation: not minor compacting closed tablet " + keyExtent); } else { log.info("Ignoring memory manager recommendation: not minor compacting " + keyExtent); @@ -443,19 +452,11 @@ public class TabletServerResourceManager { } } - public synchronized TabletResourceManager createTabletResourceManager() { - TabletResourceManager trm = new TabletResourceManager(); + public synchronized TabletResourceManager createTabletResourceManager(KeyExtent extent, AccumuloConfiguration conf) { + TabletResourceManager trm = new TabletResourceManager(extent, conf); return trm; } - synchronized private void addTabletResource(TabletResourceManager tr) { - tabletResources.add(tr); - } - - synchronized private void removeTabletResource(TabletResourceManager tr) { - tabletResources.remove(tr); - } - public class TabletResourceManager { private final long creationTime = System.currentTimeMillis(); @@ -464,20 +465,22 @@ public class TabletServerResourceManager { private volatile boolean closed = false; - private Tablet tablet; - - private AccumuloConfiguration tableConf; + private final KeyExtent extent; - TabletResourceManager() {} + private final AccumuloConfiguration tableConf; - void setTablet(Tablet tablet, AccumuloConfiguration tableConf) { - this.tablet = tablet; + TabletResourceManager(KeyExtent extent, AccumuloConfiguration tableConf) { + checkNotNull(extent, "extent is null"); + checkNotNull(tableConf, "tableConf is null"); + this.extent = extent; this.tableConf = tableConf; - // TabletResourceManager is not really initialized until this - // function is called.... so do not make it publicly available - // until now + } - addTabletResource(this); + KeyExtent getExtent() { + return extent; + } + AccumuloConfiguration getTableConfiguration() { + return tableConf; } // BEGIN methods that Tablets call to manage their set of open map files @@ -489,7 +492,7 @@ public class TabletServerResourceManager { synchronized ScanFileManager newScanFileManager() { if (closed) throw new IllegalStateException("closed"); - return fileManager.newScanFileManager(tablet.getExtent()); + return fileManager.newScanFileManager(extent); } // END methods that Tablets call to manage their set of open map files @@ -500,7 +503,7 @@ public class TabletServerResourceManager { private AtomicLong lastReportedMincSize = new AtomicLong(); private volatile long lastReportedCommitTime = 0; - public void updateMemoryUsageStats(long size, long mincSize) { + public void updateMemoryUsageStats(Tablet tablet, long size, long mincSize) { // do not want to update stats for every little change, // so only do it under certain circumstances... the reason @@ -563,7 +566,7 @@ public class TabletServerResourceManager { CompactionStrategy strategy = Property.createInstanceFromPropertyName(tableConf, Property.TABLE_COMPACTION_STRATEGY, CompactionStrategy.class, new DefaultCompactionStrategy()); strategy.init(Property.getCompactionStrategyOptions(tableConf)); - MajorCompactionRequest request = new MajorCompactionRequest(tablet.getExtent(), reason, TabletServerResourceManager.this.fs, tableConf); + MajorCompactionRequest request = new MajorCompactionRequest(extent, reason, TabletServerResourceManager.this.fs, tableConf); request.setFiles(tabletFiles); try { return strategy.shouldCompact(request); @@ -590,10 +593,8 @@ public class TabletServerResourceManager { if (openFilesReserved) throw new IOException("tired to close files while open files reserved"); - TabletServerResourceManager.this.removeTabletResource(this); - - memMgmt.tabletClosed(tablet.getExtent()); - memoryManager.tabletClosed(tablet.getExtent()); + memMgmt.tabletClosed(extent); + memoryManager.tabletClosed(extent); closed = true; } http://git-wip-us.apache.org/repos/asf/accumulo/blob/eade3b59/server/tserver/src/test/java/org/apache/accumulo/tserver/TabletResourceManagerTest.java ---------------------------------------------------------------------- diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/TabletResourceManagerTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/TabletResourceManagerTest.java new file mode 100644 index 0000000..9d9db6f --- /dev/null +++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/TabletResourceManagerTest.java @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.tserver; + +import org.apache.accumulo.core.data.KeyExtent; +import org.apache.accumulo.server.conf.TableConfiguration; +import org.apache.accumulo.tserver.TabletServerResourceManager.TabletResourceManager; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.*; +import org.easymock.Capture; +import static org.easymock.EasyMock.*; + +public class TabletResourceManagerTest { + private TabletServerResourceManager tsrm; + private TabletResourceManager trm; + private Tablet tablet; + private KeyExtent extent; + private TableConfiguration conf; + + @Before + public void setUp() throws Exception { + tsrm = createMock(TabletServerResourceManager.class); + tablet = createMock(Tablet.class); + extent = createMock(KeyExtent.class); + conf = createMock(TableConfiguration.class); + trm = tsrm.new TabletResourceManager(extent, conf); + } + + @Test + public void testConstruction() { + assertEquals(extent, trm.getExtent()); + assertEquals(conf, trm.getTableConfiguration()); + } +}
