accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubb...@apache.org
Subject [accumulo] branch master updated: Close unclosed singleton resources in admin utilities (#1586)
Date Tue, 14 Apr 2020 20:27:42 GMT
This is an automated email from the ASF dual-hosted git repository.

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


The following commit(s) were added to refs/heads/master by this push:
     new 16d46a8  Close unclosed singleton resources in admin utilities (#1586)
16d46a8 is described below

commit 16d46a85895b2aa8c2e8efa4949eac0fdef4ce55
Author: Karthick Narendran <karthick.narendran@gmail.com>
AuthorDate: Tue Apr 14 21:27:32 2020 +0100

    Close unclosed singleton resources in admin utilities (#1586)
    
    Add a mechanism to forcibly close singleton resources managed by SingletonManager,
    such as ZooSession, for use with admin utilities that did not previously close them.
    
    This fixes #1578, a bug where non-daemon ZooKeeper threads prevented SetGoalState
    from terminating properly, because the ZooKeeper client object was unclosed.
    
    Commit message re-written by Christopher Tubbs <ctubbsii@apache.org> to provide
more
    details on the nature of the change.
---
 .../accumulo/core/singletons/SingletonManager.java | 23 ++++--
 .../org/apache/accumulo/server/util/Admin.java     |  4 +
 .../org/apache/accumulo/server/util/ZooZap.java    | 86 ++++++++++++----------
 .../apache/accumulo/master/state/SetGoalState.java | 18 +++--
 4 files changed, 80 insertions(+), 51 deletions(-)

diff --git a/core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
b/core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
index 06c2cc1..1b4fe1b 100644
--- a/core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
+++ b/core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
@@ -58,14 +58,20 @@ public class SingletonManager {
      */
     CLIENT,
     /**
-     * In this mode singletons are never disabled.
+     * In this mode singletons are never disabled, unless the CLOSED mode is entered.
      */
     SERVER,
     /**
      * In this mode singletons are never disabled unless the mode is set back to CLIENT.
The user
      * can do this by using util.CleanUp (an old API created for users).
      */
-    CONNECTOR
+    CONNECTOR,
+    /**
+     * In this mode singletons are permanently disabled and entering this mode prevents
+     * transitioning to other modes.
+     */
+    CLOSED
+
   }
 
   private static long reservations;
@@ -148,6 +154,10 @@ public class SingletonManager {
    * Change how singletons are managed. The default mode is {@link Mode#CLIENT}
    */
   public static synchronized void setMode(Mode mode) {
+    if (SingletonManager.mode == mode)
+      return;
+    if (SingletonManager.mode == Mode.CLOSED)
+      throw new IllegalStateException("Cannot leave closed mode once entered");
     if (SingletonManager.mode == Mode.CLIENT && mode == Mode.CONNECTOR) {
       if (transitionedFromClientToConnector) {
         throw new IllegalStateException("Can only transition from " + Mode.CLIENT + " to
"
@@ -160,8 +170,11 @@ public class SingletonManager {
       transitionedFromClientToConnector = true;
     }
 
-    // do not change from server mode, its a terminal mode that can not be left once entered
-    if (SingletonManager.mode != Mode.SERVER) {
+    /*
+     * Always allow transition to closed and only allow transition to client/connector when
the
+     * current mode is not server.
+     */
+    if (SingletonManager.mode != Mode.SERVER || mode == Mode.CLOSED) {
       SingletonManager.mode = mode;
     }
     transition();
@@ -173,7 +186,7 @@ public class SingletonManager {
   }
 
   private static void transition() {
-    if (enabled && reservations == 0 && mode == Mode.CLIENT) {
+    if (enabled && ((reservations == 0 && mode == Mode.CLIENT) || mode ==
Mode.CLOSED)) {
       for (SingletonService service : services) {
         disable(service);
       }
diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
index 3e91a01..844387c 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
@@ -52,6 +52,8 @@ import org.apache.accumulo.core.security.Authorizations;
 import org.apache.accumulo.core.security.NamespacePermission;
 import org.apache.accumulo.core.security.SystemPermission;
 import org.apache.accumulo.core.security.TablePermission;
+import org.apache.accumulo.core.singletons.SingletonManager;
+import org.apache.accumulo.core.singletons.SingletonManager.Mode;
 import org.apache.accumulo.core.trace.TraceUtil;
 import org.apache.accumulo.core.util.AddressUtil;
 import org.apache.accumulo.core.util.HostAndPort;
@@ -275,6 +277,8 @@ public class Admin implements KeywordExecutable {
     } catch (Exception e) {
       log.error("{}", e.getMessage(), e);
       System.exit(3);
+    } finally {
+      SingletonManager.setMode(Mode.CLOSED);
     }
   }
 
diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java b/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java
index 4adaa6d..9c58ae3 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java
@@ -24,6 +24,8 @@ import org.apache.accumulo.core.Constants;
 import org.apache.accumulo.core.cli.Help;
 import org.apache.accumulo.core.conf.Property;
 import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.core.singletons.SingletonManager;
+import org.apache.accumulo.core.singletons.SingletonManager.Mode;
 import org.apache.accumulo.core.volume.VolumeConfiguration;
 import org.apache.accumulo.fate.zookeeper.ZooLock;
 import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
@@ -68,58 +70,62 @@ public class ZooZap {
       return;
     }
 
-    var siteConf = SiteConfiguration.auto();
-    Configuration hadoopConf = new Configuration();
-    // Login as the server on secure HDFS
-    if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
-      SecurityUtil.serverLogin(siteConf);
-    }
+    try {
+      var siteConf = SiteConfiguration.auto();
+      Configuration hadoopConf = new Configuration();
+      // Login as the server on secure HDFS
+      if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
+        SecurityUtil.serverLogin(siteConf);
+      }
 
-    String volDir = VolumeConfiguration.getVolumeUris(siteConf, hadoopConf).iterator().next();
-    Path instanceDir = new Path(volDir, "instance_id");
-    String iid = VolumeManager.getInstanceIDFromHdfs(instanceDir, siteConf, hadoopConf);
-    ZooReaderWriter zoo = new ZooReaderWriter(siteConf);
+      String volDir = VolumeConfiguration.getVolumeUris(siteConf, hadoopConf).iterator().next();
+      Path instanceDir = new Path(volDir, "instance_id");
+      String iid = VolumeManager.getInstanceIDFromHdfs(instanceDir, siteConf, hadoopConf);
+      ZooReaderWriter zoo = new ZooReaderWriter(siteConf);
 
-    if (opts.zapMaster) {
-      String masterLockPath = Constants.ZROOT + "/" + iid + Constants.ZMASTER_LOCK;
+      if (opts.zapMaster) {
+        String masterLockPath = Constants.ZROOT + "/" + iid + Constants.ZMASTER_LOCK;
 
-      try {
-        zapDirectory(zoo, masterLockPath, opts);
-      } catch (Exception e) {
-        e.printStackTrace();
+        try {
+          zapDirectory(zoo, masterLockPath, opts);
+        } catch (Exception e) {
+          e.printStackTrace();
+        }
       }
-    }
 
-    if (opts.zapTservers) {
-      String tserversPath = Constants.ZROOT + "/" + iid + Constants.ZTSERVERS;
-      try {
-        List<String> children = zoo.getChildren(tserversPath);
-        for (String child : children) {
-          message("Deleting " + tserversPath + "/" + child + " from zookeeper", opts);
-
-          if (opts.zapMaster) {
-            zoo.recursiveDelete(tserversPath + "/" + child, NodeMissingPolicy.SKIP);
-          } else {
-            String path = tserversPath + "/" + child;
-            if (zoo.getChildren(path).size() > 0) {
-              if (!ZooLock.deleteLock(zoo, path, "tserver")) {
-                message("Did not delete " + tserversPath + "/" + child, opts);
+      if (opts.zapTservers) {
+        String tserversPath = Constants.ZROOT + "/" + iid + Constants.ZTSERVERS;
+        try {
+          List<String> children = zoo.getChildren(tserversPath);
+          for (String child : children) {
+            message("Deleting " + tserversPath + "/" + child + " from zookeeper", opts);
+
+            if (opts.zapMaster) {
+              zoo.recursiveDelete(tserversPath + "/" + child, NodeMissingPolicy.SKIP);
+            } else {
+              String path = tserversPath + "/" + child;
+              if (zoo.getChildren(path).size() > 0) {
+                if (!ZooLock.deleteLock(zoo, path, "tserver")) {
+                  message("Did not delete " + tserversPath + "/" + child, opts);
+                }
               }
             }
           }
+        } catch (Exception e) {
+          log.error("{}", e.getMessage(), e);
         }
-      } catch (Exception e) {
-        log.error("{}", e.getMessage(), e);
       }
-    }
 
-    if (opts.zapTracers) {
-      String path = siteConf.get(Property.TRACE_ZK_PATH);
-      try {
-        zapDirectory(zoo, path, opts);
-      } catch (Exception e) {
-        // do nothing if the /tracers node does not exist.
+      if (opts.zapTracers) {
+        String path = siteConf.get(Property.TRACE_ZK_PATH);
+        try {
+          zapDirectory(zoo, path, opts);
+        } catch (Exception e) {
+          // do nothing if the /tracers node does not exist.
+        }
       }
+    } finally {
+      SingletonManager.setMode(Mode.CLOSED);
     }
 
   }
diff --git a/server/master/src/main/java/org/apache/accumulo/master/state/SetGoalState.java
b/server/master/src/main/java/org/apache/accumulo/master/state/SetGoalState.java
index 40552b7..59dba2b 100644
--- a/server/master/src/main/java/org/apache/accumulo/master/state/SetGoalState.java
+++ b/server/master/src/main/java/org/apache/accumulo/master/state/SetGoalState.java
@@ -23,6 +23,8 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import org.apache.accumulo.core.Constants;
 import org.apache.accumulo.core.conf.SiteConfiguration;
 import org.apache.accumulo.core.master.thrift.MasterGoalState;
+import org.apache.accumulo.core.singletons.SingletonManager;
+import org.apache.accumulo.core.singletons.SingletonManager.Mode;
 import org.apache.accumulo.fate.zookeeper.ZooUtil.NodeExistsPolicy;
 import org.apache.accumulo.server.ServerContext;
 import org.apache.accumulo.server.ServerUtil;
@@ -40,12 +42,16 @@ public class SetGoalState {
       System.exit(-1);
     }
 
-    var context = new ServerContext(SiteConfiguration.auto());
-    SecurityUtil.serverLogin(context.getConfiguration());
-    ServerUtil.waitForZookeeperAndHdfs(context);
-    context.getZooReaderWriter().putPersistentData(
-        context.getZooKeeperRoot() + Constants.ZMASTER_GOAL_STATE, args[0].getBytes(UTF_8),
-        NodeExistsPolicy.OVERWRITE);
+    try {
+      var context = new ServerContext(SiteConfiguration.auto());
+      SecurityUtil.serverLogin(context.getConfiguration());
+      ServerUtil.waitForZookeeperAndHdfs(context);
+      context.getZooReaderWriter().putPersistentData(
+          context.getZooKeeperRoot() + Constants.ZMASTER_GOAL_STATE, args[0].getBytes(UTF_8),
+          NodeExistsPolicy.OVERWRITE);
+    } finally {
+      SingletonManager.setMode(Mode.CLOSED);
+    }
   }
 
 }


Mime
View raw message