Author: tomekr
Date: Tue Nov 22 11:20:41 2016
New Revision: 1770823
URL: http://svn.apache.org/viewvc?rev=1770823&view=rev
Log:
OAK-5142: Make sure that metadata entries are eventually removed
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/CacheMetadata.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/NodeCache.java
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/CacheMetadata.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/CacheMetadata.java?rev=1770823&r1=1770822&r2=1770823&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/CacheMetadata.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/CacheMetadata.java
Tue Nov 22 11:20:41 2016
@@ -21,6 +21,37 @@ import java.util.concurrent.atomic.Atomi
import static com.google.common.collect.Maps.newConcurrentMap;
+/**
+ * In order to avoid leaking values from the metadataMap, following order
should
+ * be maintained for combining the cache and CacheMetadata:
+ *
+ * 1. For remove(), removeAll() and clear():
+ *
+ * - cache.invalidate()
+ * - metadata.remove()
+ *
+ * 2. For put(), putAll() and putFromPersistenceAndIncrement():
+ *
+ * - metadata.put()
+ * - cache.put()
+ *
+ * 3. For increment():
+ *
+ * - metadata.increment()
+ * - cache.get()
+ * - (metadata.remove() if value doesn't exists in cache)
+ *
+ * 4. For incrementAll():
+ *
+ * - metadata.incrementAll()
+ * - cache.getAll()
+ * - (metadata.removeAll() on keys that returned nulls)
+ *
+ * Preserving this order will allow to avoid leaked values in the metadata
without
+ * an extra synchronization between cache and metadata operations. This
strategy
+ * is a best-effort option - it may happen that cache values won't have their
+ * metadata entries.
+ */
public class CacheMetadata<K> {
private final ConcurrentMap<K, MetadataEntry> metadataMap =
newConcurrentMap();
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/NodeCache.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/NodeCache.java?rev=1770823&r1=1770822&r2=1770823&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/NodeCache.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/persistentCache/NodeCache.java
Tue Nov 22 11:20:41 2016
@@ -16,9 +16,12 @@
*/
package org.apache.jackrabbit.oak.plugins.document.persistentCache;
+import static com.google.common.base.Predicates.in;
+import static com.google.common.base.Predicates.not;
import static com.google.common.cache.RemovalCause.COLLECTED;
import static com.google.common.cache.RemovalCause.EXPIRED;
import static com.google.common.cache.RemovalCause.SIZE;
+import static com.google.common.collect.Iterables.filter;
import static java.util.Collections.singleton;
import java.nio.ByteBuffer;
@@ -31,6 +34,8 @@ import java.util.concurrent.ExecutionExc
import javax.annotation.Nullable;
+import com.google.common.base.Predicates;
+import com.google.common.collect.Iterables;
import org.apache.jackrabbit.oak.plugins.document.DocumentNodeStore;
import org.apache.jackrabbit.oak.plugins.document.DocumentStore;
import
org.apache.jackrabbit.oak.plugins.document.persistentCache.PersistentCache.GenerationCache;
@@ -170,17 +175,19 @@ class NodeCache<K, V> implements Cache<K
@Override
@Nullable
public V getIfPresent(Object key) {
+ memCacheMetadata.increment((K) key);
V value = memCache.getIfPresent(key);
- if (value != null) {
- memCacheMetadata.increment((K) key);
+ if (value == null) {
+ memCacheMetadata.remove(key);
+ } else {
return value;
}
stats.markRequest();
value = readIfPresent((K) key);
if (value != null) {
- memCache.put((K) key, value);
memCacheMetadata.putFromPersistenceAndIncrement((K) key);
+ memCache.put((K) key, value);
stats.markHit();
}
return value;
@@ -200,8 +207,8 @@ class NodeCache<K, V> implements Cache<K
// Track entry load time
TimerStats.Context ctx = stats.startLoaderTimer();
try {
- value = memCache.get(key, valueLoader);
memCacheMetadata.increment(key);
+ value = memCache.get(key, valueLoader);
ctx.stop();
if (!async) {
write((K) key, value);
@@ -217,15 +224,17 @@ class NodeCache<K, V> implements Cache<K
@Override
public ImmutableMap<K, V> getAllPresent(
Iterable<?> keys) {
- ImmutableMap<K, V> result = memCache.getAllPresent(keys);
+ Iterable<K> typedKeys = (Iterable<K>) keys;
memCacheMetadata.incrementAll(keys);
+ ImmutableMap<K, V> result = memCache.getAllPresent(keys);
+ memCacheMetadata.removeAll(filter(typedKeys,
not(in(result.keySet()))));
return result;
}
@Override
public void put(K key, V value) {
- memCache.put(key, value);
memCacheMetadata.put(key);
+ memCache.put(key, value);
if (!async) {
write((K) key, value);
}
@@ -248,8 +257,8 @@ class NodeCache<K, V> implements Cache<K
@Override
public void putAll(Map<? extends K, ? extends V> m) {
- memCache.putAll(m);
memCacheMetadata.putAll(m.keySet());
+ memCache.putAll(m);
}
@Override
@@ -298,8 +307,8 @@ class NodeCache<K, V> implements Cache<K
memCacheMetadata.remove(key);
} else {
value = (V) valueType.read(buff);
- memCache.put(key, value);
memCacheMetadata.put(key);
+ memCache.put(key, value);
}
stats.markRecvBroadcast();
if (!async) {
