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