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

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

commit 8d3bcdaf345af2a57666b148bf1176561fa797e9
Author: Keith Turner <ktur...@apache.org>
AuthorDate: Fri Feb 2 12:13:59 2018 -0500

    Revert "ACCUMULO-4779 Avoid locks in ZooCache when data in cache"
    
    This reverts commit cf9e754b045e3fac452df282bcf4ec97974038a0.
---
 .../apache/accumulo/fate/zookeeper/ZooCache.java   | 97 ++++++----------------
 1 file changed, 26 insertions(+), 71 deletions(-)

diff --git 
a/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java 
b/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
index 0c82d41..ba837f8 100644
--- a/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
+++ b/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
@@ -28,7 +28,6 @@ import java.util.ConcurrentModificationException;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.LockSupport;
 import java.util.concurrent.locks.ReadWriteLock;
@@ -44,7 +43,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableList;
 
 /**
  * A cache for values stored in ZooKeeper. Values are kept up to date as they 
change.
@@ -65,38 +63,6 @@ public class ZooCache {
 
   private final ZooReader zReader;
 
-  private static class ImmutableCacheCopies {
-    final Map<String,byte[]> cache;
-    final Map<String,Stat> statCache;
-    final Map<String,List<String>> childrenCache;
-
-    ImmutableCacheCopies() {
-      cache = Collections.emptyMap();
-      statCache = Collections.emptyMap();
-      childrenCache = Collections.emptyMap();
-    }
-
-    ImmutableCacheCopies(Map<String,byte[]> cache, Map<String,Stat> statCache, 
Map<String,List<String>> childrenCache) {
-      this.cache = Collections.unmodifiableMap(new HashMap<>(cache));
-      this.statCache = Collections.unmodifiableMap(new HashMap<>(statCache));
-      this.childrenCache = Collections.unmodifiableMap(new 
HashMap<>(childrenCache));
-    }
-
-    ImmutableCacheCopies(ImmutableCacheCopies prev, Map<String,List<String>> 
childrenCache) {
-      this.cache = prev.cache;
-      this.statCache = prev.statCache;
-      this.childrenCache = Collections.unmodifiableMap(new 
HashMap<>(childrenCache));
-    }
-
-    ImmutableCacheCopies(Map<String,byte[]> cache, Map<String,Stat> statCache, 
ImmutableCacheCopies prev) {
-      this.cache = Collections.unmodifiableMap(new HashMap<>(cache));
-      this.statCache = Collections.unmodifiableMap(new HashMap<>(statCache));
-      this.childrenCache = prev.childrenCache;
-    }
-  }
-
-  private volatile ImmutableCacheCopies immutableCache = new 
ImmutableCacheCopies();
-
   /**
    * Returns a ZooKeeper session. Calls should be made within run of 
ZooRunnable after caches are checked. This will be performed at each retry of 
the run
    * method. Calls to {@link #getZooKeeper()} should be made, ideally, after 
cache checks since other threads may have succeeded when updating the cache. 
Doing
@@ -257,17 +223,19 @@ public class ZooCache {
    *          path of node
    * @return children list, or null if node has no children or does not exist
    */
-  public List<String> getChildren(final String zPath) {
+  public synchronized List<String> getChildren(final String zPath) {
 
     ZooRunnable<List<String>> zr = new ZooRunnable<List<String>>() {
 
       @Override
       public List<String> run() throws KeeperException, InterruptedException {
-
-        // only read volatile once for consistency
-        ImmutableCacheCopies lic = immutableCache;
-        if (lic.childrenCache.containsKey(zPath)) {
-          return lic.childrenCache.get(zPath);
+        try {
+          cacheReadLock.lock();
+          if (childrenCache.containsKey(zPath)) {
+            return childrenCache.get(zPath);
+          }
+        } finally {
+          cacheReadLock.unlock();
         }
 
         cacheWriteLock.lock();
@@ -279,11 +247,7 @@ public class ZooCache {
           final ZooKeeper zooKeeper = getZooKeeper();
 
           List<String> children = zooKeeper.getChildren(zPath, watcher);
-          if (children != null) {
-            children = ImmutableList.copyOf(children);
-          }
           childrenCache.put(zPath, children);
-          immutableCache = new ImmutableCacheCopies(immutableCache, 
childrenCache);
           return children;
         } catch (KeeperException ke) {
           if (ke.code() != Code.NONODE) {
@@ -297,7 +261,12 @@ public class ZooCache {
 
     };
 
-    return zr.retry();
+    List<String> children = zr.retry();
+
+    if (children == null) {
+      return null;
+    }
+    return Collections.unmodifiableList(children);
   }
 
   /**
@@ -326,16 +295,15 @@ public class ZooCache {
       @Override
       public byte[] run() throws KeeperException, InterruptedException {
         Stat stat = null;
-
-        // only read volatile once so following code works with a consistent 
snapshot
-        ImmutableCacheCopies lic = immutableCache;
-        byte[] val = lic.cache.get(zPath);
-        if (val != null || lic.cache.containsKey(zPath)) {
-          if (status != null) {
-            stat = lic.statCache.get(zPath);
+        cacheReadLock.lock();
+        try {
+          if (cache.containsKey(zPath)) {
+            stat = statCache.get(zPath);
             copyStats(status, stat);
+            return cache.get(zPath);
           }
-          return val;
+        } finally {
+          cacheReadLock.unlock();
         }
 
         /*
@@ -366,6 +334,7 @@ public class ZooCache {
           }
           put(zPath, data, stat);
           copyStats(status, stat);
+          statCache.put(zPath, stat);
           return data;
         } finally {
           cacheWriteLock.unlock();
@@ -408,8 +377,6 @@ public class ZooCache {
     try {
       cache.put(zPath, data);
       statCache.put(zPath, stat);
-
-      immutableCache = new ImmutableCacheCopies(cache, statCache, 
immutableCache);
     } finally {
       cacheWriteLock.unlock();
     }
@@ -421,8 +388,6 @@ public class ZooCache {
       cache.remove(zPath);
       childrenCache.remove(zPath);
       statCache.remove(zPath);
-
-      immutableCache = new ImmutableCacheCopies(cache, statCache, 
childrenCache);
     } finally {
       cacheWriteLock.unlock();
     }
@@ -431,14 +396,12 @@ public class ZooCache {
   /**
    * Clears this cache.
    */
-  public void clear() {
+  public synchronized void clear() {
     cacheWriteLock.lock();
     try {
       cache.clear();
       childrenCache.clear();
       statCache.clear();
-
-      immutableCache = new ImmutableCacheCopies();
     } finally {
       cacheWriteLock.unlock();
     }
@@ -452,14 +415,8 @@ public class ZooCache {
    * @return true if data value is cached
    */
   @VisibleForTesting
-  boolean dataCached(String zPath) {
-    cacheReadLock.lock();
-    try {
-      return immutableCache.cache.containsKey(zPath) && 
cache.containsKey(zPath);
-    } finally {
-      cacheReadLock.unlock();
-    }
-
+  synchronized boolean dataCached(String zPath) {
+    return cache.containsKey(zPath);
   }
 
   /**
@@ -473,7 +430,7 @@ public class ZooCache {
   boolean childrenCached(String zPath) {
     cacheReadLock.lock();
     try {
-      return immutableCache.childrenCache.containsKey(zPath) && 
childrenCache.containsKey(zPath);
+      return childrenCache.containsKey(zPath);
     } finally {
       cacheReadLock.unlock();
     }
@@ -505,8 +462,6 @@ public class ZooCache {
         if (path.startsWith(zPath))
           i.remove();
       }
-
-      immutableCache = new ImmutableCacheCopies(cache, statCache, 
childrenCache);
     } finally {
       cacheWriteLock.unlock();
     }

-- 
To stop receiving notification emails like this one, please contact
ktur...@apache.org.

Reply via email to