Remove metrics when cache gets closed
When plugin with persistent caches gets reloaded it fails to load with
the following exception:
java.lang.IllegalArgumentException: A metric named cache/chroniclemap/percentage_free_space_test-cache-plugin_test_cache already exists
...
The problem is caused by cache related metrics that are created when
cache-chroniclemap instantiates the persistent cache implementation for
plugin that is being reloaded.
This change ensures that all cache related metrics are removed when cache is
closed.
Notes:
* This is the necessary but not sufficient step to solve #15587.
It is enough to solve the happy path when 'Gerrit-ReloadMode: restart'.
IOW when the old version of plugin with persistent caches gets unloaded
before the new version is loaded.
* Description on how to reload plugin with persistent caches when
cache-chroniclemap lib-module is installed was added to migration.md
Bug: Issue 15587
Depends-On: I06d48535702a8acd0bcb4e1315a35b3c1d1768cb
Change-Id: I03e34dd3463fad00ba8fbc174597c896a93db547
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
index c9aff80..0730380 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
@@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.flogger.FluentLogger;
import com.google.errorprone.annotations.CompatibleWith;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
@@ -83,6 +84,7 @@
}
private class Metrics {
+ private final RegistrationHandle indexSize;
private final Timer0 addLatency;
private final Timer0 removeAndConsumeOlderThanLatency;
private final Timer0 removeAndConsumeLruKeyLatency;
@@ -94,14 +96,15 @@
private Metrics(MetricMaker metricMaker, String name) {
String sanitizedName = metricMaker.sanitizeMetricName(name);
- metricMaker.newCallbackMetric(
- "cache/chroniclemap/keys_index_size_" + sanitizedName,
- Integer.class,
- new Description(
- String.format(
- "The number of cache index keys for %s cache that are currently in memory",
- name)),
- keys::size);
+ indexSize =
+ metricMaker.newCallbackMetric(
+ "cache/chroniclemap/keys_index_size_" + sanitizedName,
+ Integer.class,
+ new Description(
+ String.format(
+ "The number of cache index keys for %s cache that are currently in memory",
+ name)),
+ keys::size);
addLatency =
metricMaker.newTimer(
@@ -170,6 +173,17 @@
.setCumulative()
.setUnit("errors"));
}
+
+ private void close() {
+ indexSize.remove();
+ addLatency.remove();
+ removeAndConsumeOlderThanLatency.remove();
+ removeAndConsumeLruKeyLatency.remove();
+ restoreLatency.remove();
+ persistLatency.remove();
+ restoreFailures.remove();
+ persistFailures.remove();
+ }
}
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -337,6 +351,11 @@
}
}
+ void close() {
+ persist();
+ metrics.close();
+ }
+
static final File tempIndexFile(File indexFile) {
return new File(String.format("%s.tmp", indexFile.getPath()));
}
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 1d60084..7efc8b5 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
@@ -110,7 +110,9 @@
}
mapBuilder.averageValueSize(config.getAverageValueSize());
- mapBuilder.valueMarshaller(new TimedValueMarshaller<>(metricMaker, def.name()));
+
+ TimedValueMarshaller<V> valueMarshaller = new TimedValueMarshaller<>(metricMaker, def.name());
+ mapBuilder.valueMarshaller(valueMarshaller);
mapBuilder.entries(config.getMaxEntries());
@@ -138,7 +140,13 @@
store.percentageFreeSpace(),
config.getPersistIndexEvery());
- return new ChronicleMapStore<>(store, config, metricMaker);
+ return new ChronicleMapStore<K, V>(store, config, metricMaker) {
+ @Override
+ public void close() {
+ super.close();
+ valueMarshaller.close();
+ }
+ };
}
protected PersistentCacheDef<K, V> getCacheDefinition() {
@@ -347,7 +355,7 @@
public void close() {
store.close();
- keysIndex.persist();
+ keysIndex.close();
}
public double percentageUsedAutoResizes() {
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStore.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStore.java
index 0419ca4..38759f4 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStore.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStore.java
@@ -320,6 +320,7 @@
@Override
public void close() {
store.close();
+ metrics.close();
}
@Override
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
index cf758b5..c27be99 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
@@ -14,15 +14,19 @@
package com.googlesource.gerrit.modules.cache.chroniclemap;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
+import java.util.HashSet;
+import java.util.Set;
class ChronicleMapStoreMetrics {
private final String sanitizedName;
private final MetricMaker metricMaker;
private final String name;
private final Counter0 storePutFailures;
+ private final Set<RegistrationHandle> callbacks;
ChronicleMapStoreMetrics(String name, MetricMaker metricMaker) {
this.name = name;
@@ -37,6 +41,7 @@
+ name)
.setCumulative()
.setUnit("errors"));
+ this.callbacks = new HashSet<>(3, 1.0F);
}
void incrementPutFailures() {
@@ -50,27 +55,37 @@
"cache/chroniclemap/remaining_autoresizes_" + sanitizedName;
String MAX_AUTORESIZES_METRIC = "cache/chroniclemap/max_autoresizes_" + sanitizedName;
- metricMaker.newCallbackMetric(
- PERCENTAGE_FREE_SPACE_METRIC,
- Long.class,
- new Description(
- String.format("The amount of free space in the %s cache as a percentage", name)),
- () -> (long) store.percentageFreeSpace());
+ callbacks.add(
+ metricMaker.newCallbackMetric(
+ PERCENTAGE_FREE_SPACE_METRIC,
+ Long.class,
+ new Description(
+ String.format("The amount of free space in the %s cache as a percentage", name)),
+ () -> (long) store.percentageFreeSpace()));
- metricMaker.newCallbackMetric(
- REMAINING_AUTORESIZES_METRIC,
- Integer.class,
- new Description(
- String.format(
- "The number of times the %s cache can automatically expand its capacity", name)),
- store::remainingAutoResizes);
+ callbacks.add(
+ metricMaker.newCallbackMetric(
+ REMAINING_AUTORESIZES_METRIC,
+ Integer.class,
+ new Description(
+ String.format(
+ "The number of times the %s cache can automatically expand its capacity",
+ name)),
+ store::remainingAutoResizes));
- metricMaker.newConstantMetric(
- MAX_AUTORESIZES_METRIC,
- store.maxAutoResizes(),
- new Description(
- String.format(
- "The maximum number of times the %s cache can automatically expand its capacity",
- name)));
+ callbacks.add(
+ metricMaker.newConstantMetric(
+ MAX_AUTORESIZES_METRIC,
+ store.maxAutoResizes(),
+ new Description(
+ String.format(
+ "The maximum number of times the %s cache can automatically expand its capacity",
+ name))));
+ }
+
+ void close() {
+ storePutFailures.remove();
+ callbacks.forEach(RegistrationHandle::remove);
+ callbacks.clear();
}
}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
index b49a65d..098e8c6 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
@@ -79,4 +79,10 @@
metricsCache.put(name, metrics);
return metrics;
}
+
+ void close() {
+ metrics.deserializeLatency.remove();
+ metrics.serializeLatency.remove();
+ metricsCache.remove(name);
+ }
}
diff --git a/src/main/resources/Documentation/migration.md b/src/main/resources/Documentation/migration.md
index 261f08f..f0aed00 100644
--- a/src/main/resources/Documentation/migration.md
+++ b/src/main/resources/Documentation/migration.md
@@ -103,4 +103,45 @@
* size-multiplier=MULTIPLIER
Multiplicative factor for the number of entries allowed in chronicle-map.
-*default:3*
\ No newline at end of file
+*default:3*
+
+
+### Reloading plugins with persistent caches backed by chroniclemap
+
+When chroniclemap store is initiated for a cache it locks exclusively the
+underlying file and keeps it until store is closed. Store is closed when Gerrit
+is stopped (for core caches) or when plugin is unloaded through either REST or
+SSH command. The later is problematic from the plugin `reload` command
+perspective as by default it unloads old version of plugin once new version is
+successfully loaded. Considering that old version holds the lock until it gets
+unloaded new version load will not succeed. As a result the following (or
+similar) error is visible in the log:
+
+```
+[2022-08-31T17:37:56.481+02:00] [SSH gerrit plugin reload test-cache-plugin (admin)] WARN com.google.gerrit.server.plugins.PluginLoader : Cannot reload plugin test-cache-plugin
+com.google.inject.CreationException: Unable to create injector, see the following errors:
+
+1) [Guice/ErrorInCustomProvider]: ChronicleHashRecoveryFailedException: ChronicleFileLockException: Unable to acquire an exclusive file lock for gerrit/cache/test-cache-plugin.test_cache_0.dat. Make sure no other process is using the map.
+ at CacheModule.bindCache(CacheModule.java:188)
+ \_ installed by: Module -> TestCache$1
+ while locating Cache<String, String> annotated with @Named(value="test_cache")
+
+Learn more:
+ https://github.com/google/guice/wiki/ERROR_IN_CUSTOM_PROVIDER
+Caused by: ChronicleHashRecoveryFailedException: ChronicleFileLockException: Unable to acquire an exclusive file lock for gerrit/cache/test-cache-plugin.test_cache_0.dat. Make sure no other process is using the map.
+ at ChronicleMapBuilder.openWithExistingFile(ChronicleMapBuilder.java:1937)
+ at ChronicleMapBuilder.createWithFile(ChronicleMapBuilder.java:1706)
+ at ChronicleMapBuilder.recoverPersistedTo(ChronicleMapBuilder.java:1622)
+ ...
+```
+
+The following steps can be used in order to perform reload operation:
+
+1. perform the plugin `reload` in two steps by calling `remove` first and
+ following it with `add` command - the easiest way that doesn't require any
+ code modification
+
+2. add `Gerrit-ReloadMode: restart` to plugin's manifest so the when the plugin
+ `reload` command is called Gerrit unloads the old version prior loading the
+ new one - requires plugin's sources modification and build which might be
+ not an option in certain cases
\ No newline at end of file