Do not throw when failing to store into chronicle-map
When storing a new entry in chronicle-map it is possible that the
operation might fail.
For instance, if the new entry diverges greatly from the configured
average key and value size, the available memory chunks might not be
enough to accommodate the new entry.
Chronicle-map fails with:
```
Entry is too large: requires X chunks, Y is the maximum.
```
Another typical case would be that the cache already reached the maximum
number of auto-resizes and thus it is not able to expand anymore.
Chronicle-map fails with:
```
Attempt to allocate #X extra segment tier, Y is maximum.
```
In these cases, as well as other possible failures, we don't want the
exception to bubble up to the stack and cause the caller to fail.
The caller might be a REST API initiated by the UI via ajax, which in
turn, would cause the UI to stop rendering and a bad user experience.
Catch chronicle-map's `put()` exception locally and simply avoid caching
the offending entry.
Bug: Issue 15483
Change-Id: Ia1811d66b676388f15cb6f42ef3ee4fd5d1aa94b
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java
index 76ae8a4..181192e 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheImpl.java
@@ -285,8 +285,9 @@
public void putUnchecked(Object key, Object value, Timestamp created) {
TimedValue<?> wrappedValue = new TimedValue<>(value, created.toInstant().toEpochMilli());
KeyWrapper<?> wrappedKey = new KeyWrapper<>(key);
- store.put((KeyWrapper<K>) wrappedKey, (TimedValue<V>) wrappedValue);
- mem.put((K) key, (TimedValue<V>) wrappedValue);
+ if (tryPut(store, (KeyWrapper<K>) wrappedKey, (TimedValue<V>) wrappedValue)) {
+ mem.put((K) key, (TimedValue<V>) wrappedValue);
+ }
}
/**
@@ -301,21 +302,52 @@
*/
@SuppressWarnings("unchecked")
public void putUnchecked(KeyWrapper<Object> wrappedKey, TimedValue<Object> wrappedValue) {
- store.put((KeyWrapper<K>) wrappedKey, (TimedValue<V>) wrappedValue);
- mem.put((K) wrappedKey.getValue(), (TimedValue<V>) wrappedValue);
+ if (tryPut(store, (KeyWrapper<K>) wrappedKey, (TimedValue<V>) wrappedValue)) {
+ mem.put((K) wrappedKey.getValue(), (TimedValue<V>) wrappedValue);
+ }
}
@Override
public void put(K key, V val) {
TimedValue<V> timedVal = new TimedValue<>(val);
- mem.put(key, timedVal);
- putTimedToStore(key, timedVal);
+ if (putTimedToStore(key, timedVal)) {
+ mem.put(key, timedVal);
+ }
}
- void putTimedToStore(K key, TimedValue<V> timedVal) {
+ boolean putTimedToStore(K key, TimedValue<V> timedVal) {
KeyWrapper<K> wrappedKey = new KeyWrapper<>(key);
- store.put(wrappedKey, timedVal);
- hotEntries.add(key);
+ boolean putSuccess = tryPut(store, wrappedKey, timedVal);
+ if (putSuccess) {
+ hotEntries.add(key);
+ }
+ return putSuccess;
+ }
+
+ /**
+ * Attempt to put the key/value pair into the chronicle-map cache. Also catches and warns on disk
+ * allocation errors, so that such failures result in non-cached entries rather than throwing.
+ *
+ * @param store the chronicle-map store
+ * @param wrappedKey the wrapped key value
+ * @param timedVal the timed value
+ * @param <K> the type of the wrapped key
+ * @param <V> the type of the timed value
+ * @return true when the value was successfully inserted in chronicle-map, false otherwise
+ */
+ static <K, V> boolean tryPut(
+ ChronicleMap<KeyWrapper<K>, TimedValue<V>> store,
+ KeyWrapper<K> wrappedKey,
+ TimedValue<V> timedVal) {
+ try {
+ store.put(wrappedKey, timedVal);
+ } catch (IllegalArgumentException | IllegalStateException e) {
+ logger.atWarning().withCause(e).log(
+ "[cache %s] Caught exception when inserting entry '%s' in chronicle-map",
+ store.name(), wrappedKey.getValue());
+ return false;
+ }
+ return true;
}
public void prune() {
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheLoader.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheLoader.java
index 1da3257..66f214f 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheLoader.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheLoader.java
@@ -14,6 +14,8 @@
package com.googlesource.gerrit.modules.cache.chroniclemap;
+import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheImpl.tryPut;
+
import com.google.common.cache.CacheLoader;
import com.google.common.cache.CacheStats;
import com.google.common.flogger.FluentLogger;
@@ -96,9 +98,16 @@
missCount.increment();
long start = System.nanoTime();
TimedValue<V> loadedValue = new TimedValue<>(loader.get().load(key));
- loadSuccessCount.increment();
totalLoadTime.add(System.nanoTime() - start);
- storePersistenceExecutor.execute(() -> store.put(new KeyWrapper<>(key), loadedValue));
+ storePersistenceExecutor.execute(
+ () -> {
+ // Note that we return a loadedValue, even when we
+ // we fail populating the cache with it, to make clients more
+ // resilient to storage cache failures
+ if (tryPut(store, new KeyWrapper<>(key), loadedValue)) {
+ loadSuccessCount.increment();
+ }
+ });
return loadedValue;
}
@@ -133,8 +142,9 @@
new FutureCallback<V>() {
@Override
public void onSuccess(V result) {
- store.put(new KeyWrapper<>(key), new TimedValue<>(result));
- loadSuccessCount.increment();
+ if (tryPut(store, new KeyWrapper<>(key), new TimedValue<>(result))) {
+ loadSuccessCount.increment();
+ }
totalLoadTime.add(System.nanoTime() - start);
}
@@ -175,7 +185,7 @@
@Override
public void put(K key, TimedValue<V> value) {
- store.put(new KeyWrapper<>(key), value);
+ tryPut(store, new KeyWrapper<>(key), value);
}
@Override
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
index 6cc4c05..a3a75b6 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
@@ -355,11 +355,46 @@
while (!cache.runningOutOfFreeSpace()) {
cache.put(UUID.randomUUID().toString(), UUID.randomUUID().toString());
}
- assertThat(cache.runningOutOfFreeSpace()).isTrue();
+ }
- cache.prune();
+ @Test
+ public void shouldRecoverWhenPutFailsBecauseEntryIsTooBig() throws Exception {
+ String key = UUID.randomUUID().toString();
+ String value = UUID.randomUUID().toString();
+ int uuidSize = valueSize(value);
+ gerritConfig.setInt("cache", testCacheName, "maxEntries", 1);
+ gerritConfig.setInt("cache", testCacheName, "maxBloatFactor", 1);
+ gerritConfig.setInt("cache", testCacheName, "avgKeySize", uuidSize / 2);
+ gerritConfig.setInt("cache", testCacheName, "avgValueSize", uuidSize / 2);
+ gerritConfig.save();
- assertThat(cache.runningOutOfFreeSpace()).isFalse();
+ ChronicleMapCacheImpl<String, String> cache = newCacheWithoutLoader();
+
+ cache.put(key, value);
+
+ assertThat(cache.getStore().size()).isEqualTo(0);
+ assertThat(cache.getIfPresent(key)).isNull();
+ }
+
+ @Test
+ public void shouldRecoverWhenPutFailsBecauseCacheCannotExpand() throws Exception {
+ String key = UUID.randomUUID().toString();
+ String value = UUID.randomUUID().toString();
+ int uuidSize = valueSize(value);
+ gerritConfig.setInt("cache", testCacheName, "maxEntries", 1);
+ gerritConfig.setInt("cache", testCacheName, "maxBloatFactor", 1);
+ gerritConfig.setInt("cache", testCacheName, "avgKeySize", uuidSize);
+ gerritConfig.setInt("cache", testCacheName, "avgValueSize", uuidSize);
+ gerritConfig.save();
+
+ ChronicleMapCacheImpl<String, String> cache = newCacheWithoutLoader();
+
+ cache.put(UUID.randomUUID().toString(), UUID.randomUUID().toString());
+ cache.put(UUID.randomUUID().toString(), UUID.randomUUID().toString());
+ cache.put(key, value);
+
+ assertThat(cache.getStore().size()).isEqualTo(2);
+ assertThat(cache.getIfPresent(key)).isNull();
}
@Test
