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