accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bhava...@apache.org
Subject git commit: ACCUMULO-1948 Tablet constructor no longer leaks this
Date Tue, 04 Feb 2014 20:02:47 GMT
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 <bhavanki@cloudera.com>
Authored: Mon Jan 27 17:09:39 2014 -0500
Committer: Bill Havanki <bhavanki@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());
+  }
+}


Mime
View raw message