[accumulo] branch 1.9 updated: fixes #558 use copy to avoid deadlock in tserver (#559)

[kt...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

kturner pushed a commit to branch 1.9
in repository https://gitbox.apache.org/repos/asf/accumulo.git


The following commit(s) were added to refs/heads/1.9 by this push:
     new 8519514  fixes #558 use copy to avoid deadlock in tserver (#559)
8519514 is described below

commit 8519514e4798f4c7dc694f2b007a78f1b4adc3b1
Author: Keith Turner <ke...@deenlo.com>
AuthorDate: Fri Jul 13 16:04:17 2018 -0400

    fixes #558 use copy to avoid deadlock in tserver (#559)
---
 .../org/apache/accumulo/tserver/tablet/Tablet.java | 64 ++++++++++++++--------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
index 4590872..4dc19f2 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
@@ -154,8 +154,11 @@ import org.apache.zookeeper.KeeperException.NoNodeException;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
+import com.google.common.base.Preconditions;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
 
 /**
  *
@@ -495,6 +498,8 @@ public class Tablet implements TabletCommitter {
             logEntry.getColumnQualifier().toString()));
       }
 
+      rebuildReferencedLogs();
+
       log.info(
           "Write-Ahead Log recovery complete for " + this.extent + " (" + entriesUsedOnTablet.get()
               + " mutations applied, " + getTabletMemory().getNumEntries() + " entries created)");
@@ -950,6 +955,8 @@ public class Tablet implements TabletCommitter {
 
   private synchronized MinorCompactionTask prepareForMinC(long flushId,
       MinorCompactionReason mincReason) {
+    Preconditions.checkState(otherLogs.isEmpty());
+    Preconditions.checkState(referencedLogs.equals(currentLogs));
     CommitSession oldCommitSession = getTabletMemory().prepareForMinC();
     otherLogs = currentLogs;
     currentLogs = new HashSet<>();
@@ -1524,9 +1531,9 @@ public class Tablet implements TabletCommitter {
 
     }
 
-    if (otherLogs.size() != 0 || currentLogs.size() != 0) {
+    if (otherLogs.size() != 0 || currentLogs.size() != 0 || referencedLogs.size() != 0) {
       String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
-          + currentLogs + "  otherLogs = " + otherLogs;
+          + currentLogs + "  otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
       log.error(msg);
       throw new RuntimeException(msg);
     }
@@ -2457,19 +2464,25 @@ public class Tablet implements TabletCommitter {
   }
 
   private Set<DfsLogger> currentLogs = new HashSet<>();
+  private Set<DfsLogger> otherLogs = Collections.emptySet();
+
+  // An immutable copy of currentLogs + otherLogs. This exists so that removeInUseLogs() does not
+  // have to get the tablet lock. See #558
+  private volatile Set<DfsLogger> referencedLogs = Collections.emptySet();
+
+  private synchronized void rebuildReferencedLogs() {
+    Builder<DfsLogger> builder = ImmutableSet.builder();
+    builder.addAll(currentLogs);
+    builder.addAll(otherLogs);
+    referencedLogs = builder.build();
+  }
 
   public void removeInUseLogs(Set<DfsLogger> candidates) {
-    synchronized (this) {
-      // remove logs related to minor compacting data
-      candidates.removeAll(otherLogs);
-      // remove logs related to tablets in memory data
-      candidates.removeAll(currentLogs);
-      // remove logs related to minor compaction file being added to the metadata table
-      candidates.removeAll(doomedLogs);
-    }
+    candidates.removeAll(referencedLogs);
   }
 
   Set<String> beginClearingUnusedLogs() {
+    Set<String> unusedLogs = new HashSet<>();
 
     ArrayList<String> otherLogsCopy = new ArrayList<>();
     ArrayList<String> currentLogsCopy = new ArrayList<>();
@@ -2478,20 +2491,26 @@ public class Tablet implements TabletCommitter {
     logLock.lock();
 
     synchronized (this) {
-      if (doomedLogs.size() > 0)
+      if (removingLogs)
         throw new IllegalStateException("Attempted to clear logs when removal of logs in progress");
 
       for (DfsLogger logger : otherLogs) {
         otherLogsCopy.add(logger.toString());
-        doomedLogs.add(logger);
+        unusedLogs.add(logger.getMeta());
       }
 
       for (DfsLogger logger : currentLogs) {
         currentLogsCopy.add(logger.toString());
-        doomedLogs.remove(logger);
+        unusedLogs.remove(logger.getMeta());
       }
 
       otherLogs = Collections.emptySet();
+      // Do NOT call rebuildReferenedLogs() here as that could cause walogs to be GCed before the
+      // minc rfile is written to metadata table (see #539). The clearing of otherLogs is reflected
+      // in refererncedLogs when finishClearingUnusedLogs() calls rebuildReferenedLogs().
+
+      if (unusedLogs.size() > 0)
+        removingLogs = true;
     }
 
     // do debug logging outside tablet lock
@@ -2503,23 +2522,20 @@ public class Tablet implements TabletCommitter {
       log.debug("Logs for current memory: " + getExtent() + " " + logger);
     }
 
-    Set<String> doomed = new HashSet<>();
-    for (DfsLogger logger : doomedLogs) {
-      log.debug("Logs to be destroyed: " + getExtent() + " " + logger.getMeta());
-      doomed.add(logger.getMeta());
+    for (String logger : unusedLogs) {
+      log.debug("Logs to be destroyed: " + getExtent() + " " + logger);
     }
 
-    return doomed;
+    return unusedLogs;
   }
 
   synchronized void finishClearingUnusedLogs() {
-    doomedLogs.clear();
+    removingLogs = false;
+    rebuildReferencedLogs();
     logLock.unlock();
   }
 
-  private Set<DfsLogger> otherLogs = Collections.emptySet();
-  // logs related to minor compaction file being added to the metadata table
-  private Set<DfsLogger> doomedLogs = new HashSet<>();
+  private boolean removingLogs = false;
 
   // this lock is basically used to synchronize writing of log info to metadata
   private final ReentrantLock logLock = new ReentrantLock();
@@ -2583,6 +2599,10 @@ public class Tablet implements TabletCommitter {
             numContained++;
         }
 
+        if (numAdded > 0) {
+          rebuildReferencedLogs();
+        }
+
         if (numAdded > 0 && numAdded != 1) {
           // expect to add all or none
           throw new IllegalArgumentException(