keith-turner closed pull request #365: ACCUMULO-4779 Avoid locks in ZooCache
when data in cache
URL: https://github.com/apache/accumulo/pull/365
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 ba837f88c6..0c82d413f6 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,6 +28,7 @@
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;
@@ -43,6 +44,7 @@
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.
@@ -63,6 +65,38 @@
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
@@ -223,19 +257,17 @@ public T retry() {
* path of node
* @return children list, or null if node has no children or does not exist
*/
- public synchronized List<String> getChildren(final String zPath) {
+ public List<String> getChildren(final String zPath) {
ZooRunnable<List<String>> zr = new ZooRunnable<List<String>>() {
@Override
public List<String> run() throws KeeperException, InterruptedException {
- try {
- cacheReadLock.lock();
- if (childrenCache.containsKey(zPath)) {
- return childrenCache.get(zPath);
- }
- } finally {
- cacheReadLock.unlock();
+
+ // only read volatile once for consistency
+ ImmutableCacheCopies lic = immutableCache;
+ if (lic.childrenCache.containsKey(zPath)) {
+ return lic.childrenCache.get(zPath);
}
cacheWriteLock.lock();
@@ -247,7 +279,11 @@ public T retry() {
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) {
@@ -261,12 +297,7 @@ public T retry() {
};
- List<String> children = zr.retry();
-
- if (children == null) {
- return null;
- }
- return Collections.unmodifiableList(children);
+ return zr.retry();
}
/**
@@ -295,15 +326,16 @@ public T retry() {
@Override
public byte[] run() throws KeeperException, InterruptedException {
Stat stat = null;
- cacheReadLock.lock();
- try {
- if (cache.containsKey(zPath)) {
- stat = statCache.get(zPath);
+
+ // 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);
copyStats(status, stat);
- return cache.get(zPath);
}
- } finally {
- cacheReadLock.unlock();
+ return val;
}
/*
@@ -334,7 +366,6 @@ public T retry() {
}
put(zPath, data, stat);
copyStats(status, stat);
- statCache.put(zPath, stat);
return data;
} finally {
cacheWriteLock.unlock();
@@ -377,6 +408,8 @@ private void put(String zPath, byte[] data, Stat stat) {
try {
cache.put(zPath, data);
statCache.put(zPath, stat);
+
+ immutableCache = new ImmutableCacheCopies(cache, statCache,
immutableCache);
} finally {
cacheWriteLock.unlock();
}
@@ -388,6 +421,8 @@ private void remove(String zPath) {
cache.remove(zPath);
childrenCache.remove(zPath);
statCache.remove(zPath);
+
+ immutableCache = new ImmutableCacheCopies(cache, statCache,
childrenCache);
} finally {
cacheWriteLock.unlock();
}
@@ -396,12 +431,14 @@ private void remove(String zPath) {
/**
* Clears this cache.
*/
- public synchronized void clear() {
+ public void clear() {
cacheWriteLock.lock();
try {
cache.clear();
childrenCache.clear();
statCache.clear();
+
+ immutableCache = new ImmutableCacheCopies();
} finally {
cacheWriteLock.unlock();
}
@@ -415,8 +452,14 @@ public synchronized void clear() {
* @return true if data value is cached
*/
@VisibleForTesting
- synchronized boolean dataCached(String zPath) {
- return cache.containsKey(zPath);
+ boolean dataCached(String zPath) {
+ cacheReadLock.lock();
+ try {
+ return immutableCache.cache.containsKey(zPath) &&
cache.containsKey(zPath);
+ } finally {
+ cacheReadLock.unlock();
+ }
+
}
/**
@@ -430,7 +473,7 @@ synchronized boolean dataCached(String zPath) {
boolean childrenCached(String zPath) {
cacheReadLock.lock();
try {
- return childrenCache.containsKey(zPath);
+ return immutableCache.childrenCache.containsKey(zPath) &&
childrenCache.containsKey(zPath);
} finally {
cacheReadLock.unlock();
}
@@ -462,6 +505,8 @@ public void clear(String zPath) {
if (path.startsWith(zPath))
i.remove();
}
+
+ immutableCache = new ImmutableCacheCopies(cache, statCache,
childrenCache);
} finally {
cacheWriteLock.unlock();
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
