[fluo] branch master updated: Remove race condition from FluoAdminImpl.numWorkers (#1090)

[jk...@apache.org]

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

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


The following commit(s) were added to refs/heads/master by this push:
     new 8b9b7eb  Remove race condition from FluoAdminImpl.numWorkers (#1090)
8b9b7eb is described below

commit 8b9b7ebf8c328b25f5554c242ed9f6fc3f7c07dd
Author: Joseph Koshakow <jk...@users.noreply.github.com>
AuthorDate: Mon Mar 9 18:20:36 2020 -0400

    Remove race condition from FluoAdminImpl.numWorkers (#1090)
    
    The pattern of checking if a ZNode exists and then getting it's
    children contains a race condition. If the ZNode is deleted after
    the existence check but before getting the children then a
    NoNodeException will be thrown instead of returning 0.
    
    Instead we skip the existence check and just return 0 when a
    NoNodeException is thrown. This will avoid the race condition.
    
    Fixes #1089
---
 .../org/apache/fluo/core/client/FluoAdminImpl.java | 10 +++---
 .../fluo/integration/client/FluoAdminImplIT.java   | 36 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java b/modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
index c705792..2930838 100644
--- a/modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
+++ b/modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
@@ -527,13 +527,13 @@ public class FluoAdminImpl implements FluoAdmin {
   public static int numWorkers(CuratorFramework curator) {
     int numWorkers = 0;
     try {
-      if (curator.checkExists().forPath(ZookeeperPath.FINDERS) != null) {
-        for (String path : curator.getChildren().forPath(ZookeeperPath.FINDERS)) {
-          if (path.startsWith(PartitionManager.ZK_FINDER_PREFIX)) {
-            numWorkers++;
-          }
+      for (String path : curator.getChildren().forPath(ZookeeperPath.FINDERS)) {
+        if (path.startsWith(PartitionManager.ZK_FINDER_PREFIX)) {
+          numWorkers++;
         }
       }
+    } catch (KeeperException.NoNodeException e) {
+      return 0;
     } catch (Exception e) {
       throw new RuntimeException(e);
     }
diff --git a/modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoAdminImplIT.java b/modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoAdminImplIT.java
index 6c34be3..cea68a2 100644
--- a/modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoAdminImplIT.java
+++ b/modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoAdminImplIT.java
@@ -37,12 +37,14 @@ import org.apache.fluo.api.client.Transaction;
 import org.apache.fluo.api.config.FluoConfiguration;
 import org.apache.fluo.api.data.Column;
 import org.apache.fluo.api.exceptions.FluoException;
+import org.apache.fluo.api.service.FluoWorker;
 import org.apache.fluo.core.client.FluoAdminImpl;
 import org.apache.fluo.core.client.FluoClientImpl;
 import org.apache.fluo.core.impl.Environment;
 import org.apache.fluo.core.oracle.OracleServer;
 import org.apache.fluo.core.util.AccumuloUtil;
 import org.apache.fluo.core.util.CuratorUtil;
+import org.apache.fluo.core.worker.FluoWorkerImpl;
 import org.apache.fluo.integration.ITBaseImpl;
 import org.apache.hadoop.io.Text;
 import org.junit.Assert;
@@ -314,4 +316,38 @@ public class FluoAdminImplIT extends ITBaseImpl {
       assertFalse(admin.oracleExists());
     }
   }
+
+  @Test
+  public void testNumWorkers() {
+    try (FluoAdminImpl admin = new FluoAdminImpl(config)) {
+      assertEquals(0, admin.numWorkers());
+
+      FluoWorker fluoWorker = new FluoWorkerImpl(config);
+      fluoWorker.start();
+      assertEquals(1, admin.numWorkers());
+
+      FluoWorker fluoWorker2 = new FluoWorkerImpl(config);
+      fluoWorker2.start();
+      assertEquals(2, admin.numWorkers());
+
+      fluoWorker2.stop();
+      assertEquals(1, admin.numWorkers());
+
+      fluoWorker.stop();
+      assertEquals(0, admin.numWorkers());
+    }
+  }
+
+
+  @Test
+  public void testNumWorkersWithMissingWorkerPath() throws Exception {
+    try (CuratorFramework curator = CuratorUtil.newAppCurator(config);
+        FluoAdminImpl admin = new FluoAdminImpl(config)) {
+      curator.start();
+      if (curator.checkExists().forPath(ZookeeperPath.FINDERS) != null) {
+        curator.delete().forPath(ZookeeperPath.FINDERS);
+      }
+      assertEquals(0, admin.numWorkers());
+    }
+  }
 }