Normalize chronicle-map configuration parameters
Chronicle-Map used to be instantiated not just for persistent caches,
but also for persistent caches that were effectively disabling
persistency by setting 'diskLimit' to zero or a negative value.
This, however was fixed in[1] and since then the memory cache factory is
used for those caches.
Chronicle-map should not be able to be instantiated without a specific
persisted file: for this reason, the persistedFile is now passed in
directly by the factory, so that chronicle-map just cannot be
instantiated without one.
Consequently, some parameters that were used before to check and
construct the file path, such as name, version and diskLimit have now
been removed.
[1]If911621de8ca9676d29850e449f14c684f73710e
Change-Id: I088be18d731371997f2af31ca3de8b80d36be18e
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
index b0b66b5..2fc54d3 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
@@ -16,64 +16,43 @@
import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.common.collect.ImmutableMap;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.SitePaths;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.File;
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
import java.time.Duration;
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
public class ChronicleMapCacheConfig {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final File persistedFile;
- private final long diskLimit;
private final long maxEntries;
private final long averageKeySize;
private final long averageValueSize;
private final Duration expireAfterWrite;
private final Duration refreshAfterWrite;
private final int maxBloatFactor;
- private final int version;
private final int percentageFreeSpaceEvictionThreshold;
private final int percentageHotKeys;
public interface Factory {
ChronicleMapCacheConfig create(
- @Assisted("Name") String name,
@Assisted("ConfigKey") String configKey,
- @Assisted("DiskLimit") long diskLimit,
+ @Assisted File persistedFile,
@Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
- @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite,
- int version);
+ @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite);
}
@AssistedInject
ChronicleMapCacheConfig(
@GerritServerConfig Config cfg,
- SitePaths site,
- @Assisted("Name") String name,
@Assisted("ConfigKey") String configKey,
- @Assisted("DiskLimit") long diskLimit,
+ @Assisted File persistedFile,
@Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
- @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite,
- @Assisted int version)
- throws IOException {
- this.version = version;
- final Path cacheDir = getCacheDir(site, cfg.getString("cache", null, "directory"));
- this.persistedFile =
- cacheDir != null
- ? cacheDir.resolve(String.format("%s_%s.dat", name, version)).toFile()
- : null;
- this.diskLimit = cfg.getLong("cache", configKey, "diskLimit", diskLimit);
+ @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite) {
+ this.persistedFile = persistedFile;
this.maxEntries =
cfg.getLong("cache", configKey, "maxEntries", Defaults.maxEntriesFor(configKey));
@@ -121,10 +100,6 @@
return percentageHotKeys;
}
- public int getVersion() {
- return version;
- }
-
public Duration getExpireAfterWrite() {
return expireAfterWrite;
}
@@ -149,29 +124,10 @@
return averageValueSize;
}
- public long getDiskLimit() {
- return diskLimit;
- }
-
public int getMaxBloatFactor() {
return maxBloatFactor;
}
- private static Path getCacheDir(SitePaths site, String name) throws IOException {
- if (name == null) {
- return null;
- }
- Path loc = site.resolve(name);
- if (!Files.exists(loc)) {
- Files.createDirectories(loc);
- }
- if (!Files.isWritable(loc)) {
- throw new IOException(String.format("Can't write to disk cache: %s", loc.toAbsolutePath()));
- }
- logger.atFine().log("Enabling disk cache %s", loc.toAbsolutePath());
- return loc;
- }
-
private static long toSeconds(@Nullable Duration duration) {
return duration != null ? duration.getSeconds() : 0;
}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
index fafcd42..491b5dc 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
@@ -31,6 +31,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.Files;
@@ -88,12 +89,10 @@
}
ChronicleMapCacheConfig config =
configFactory.create(
- in.name(),
in.configKey(),
- in.diskLimit(),
+ fileName(cacheDir, in.name(), in.version()),
in.expireAfterWrite(),
- in.refreshAfterWrite(),
- in.version());
+ in.refreshAfterWrite());
ChronicleMapCacheImpl<K, V> cache;
try {
cache = new ChronicleMapCacheImpl<>(in, config, null, metricMaker);
@@ -114,12 +113,10 @@
}
ChronicleMapCacheConfig config =
configFactory.create(
- in.name(),
in.configKey(),
- in.diskLimit(),
+ fileName(cacheDir, in.name(), in.version()),
in.expireAfterWrite(),
- in.refreshAfterWrite(),
- in.version());
+ in.refreshAfterWrite());
ChronicleMapCacheImpl<K, V> cache;
try {
cache = new ChronicleMapCacheImpl<>(in, config, loader, metricMaker);
@@ -161,6 +158,10 @@
cleanup.shutdownNow();
}
+ public static File fileName(Path cacheDir, String name, Integer version) {
+ return cacheDir.resolve(String.format("%s_%s.dat", name, version)).toFile();
+ }
+
private static Path getCacheDir(SitePaths site, String name) {
if (name == null) {
return null;
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 2a21121..999775d 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
@@ -96,7 +96,7 @@
+ " %s bytes|entries: %s|maxBloatFactor: %s|remainingAutoResizes:"
+ " %s|percentageFreeSpace: %s",
def.name(),
- config.getVersion(),
+ def.version(),
mapBuilder.constantlySizedKeys() ? "CONSTANT" : config.getAverageKeySize(),
config.getAverageValueSize(),
config.getMaxEntries(),
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
index d2a830b..8cb28c1 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
@@ -23,8 +23,7 @@
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.Defaults.DEFAULT_PERCENTAGE_HOT_KEYS;
import com.google.gerrit.server.config.SitePaths;
-import java.io.IOException;
-import java.nio.file.FileSystemException;
+import java.io.File;
import java.nio.file.Files;
import java.time.Duration;
import org.eclipse.jgit.lib.StoredConfig;
@@ -37,9 +36,9 @@
public class ChronicleMapCacheConfigTest {
+ private final String cacheDirectory = ".";
private final String cacheName = "foobar-cache";
private final String cacheKey = "foobar-cache-key";
- private final long definitionDiskLimit = 100;
private final int version = 1;
private final Duration expireAfterWrite = Duration.ofSeconds(10_000);
private final Duration refreshAfterWrite = Duration.ofSeconds(20_000);
@@ -57,14 +56,12 @@
new FileBasedConfig(
sitePaths.resolve("etc").resolve("gerrit.config").toFile(), FS.DETECTED);
gerritConfig.load();
+ gerritConfig.setString("cache", null, "directory", cacheDirectory);
+ gerritConfig.save();
}
@Test
- public void shouldProvidePersistedFileWhenCacheDirIsConfigured() throws Exception {
- final String directory = "cache-dir";
- gerritConfig.setString("cache", null, "directory", directory);
- gerritConfig.save();
-
+ public void shouldProvidePersistedFile() throws Exception {
assertThat(
configUnderTest(gerritConfig)
.getPersistedFile()
@@ -72,26 +69,7 @@
.getParent()
.toRealPath()
.toString())
- .isEqualTo(sitePaths.resolve(directory).toRealPath().toString());
- }
-
- @Test
- public void shouldNotProvidePersistedFileWhenCacheDirIsNotConfigured() throws Exception {
- assertThat(configUnderTest(gerritConfig).getPersistedFile()).isNull();
- }
-
- @Test
- public void shouldProvideConfiguredDiskLimitWhenDefined() throws Exception {
- long configuredDiskLimit = 50;
- gerritConfig.setLong("cache", cacheKey, "diskLimit", configuredDiskLimit);
- gerritConfig.save();
-
- assertThat(configUnderTest(gerritConfig).getDiskLimit()).isEqualTo(configuredDiskLimit);
- }
-
- @Test
- public void shouldProvideDefinitionDiskLimitWhenNotConfigured() throws Exception {
- assertThat(configUnderTest(gerritConfig).getDiskLimit()).isEqualTo(definitionDiskLimit);
+ .isEqualTo(sitePaths.resolve(cacheDirectory).toRealPath().toString());
}
@Test
@@ -178,23 +156,6 @@
}
@Test
- public void shouldThrowExceptionWhenDirectoryDoesntExist() throws Exception {
- gerritConfig.setString("cache", null, "directory", "/var/bar/foobar");
- gerritConfig.save();
-
- assertThrows(FileSystemException.class, () -> configUnderTest(gerritConfig));
- }
-
- @Test
- public void shouldThrowExceptionWhenDirectoryIsNotWriteable() throws Exception {
- gerritConfig.setString("cache", null, "directory", "/var");
- gerritConfig.save();
-
- IOException thrown = assertThrows(IOException.class, () -> configUnderTest(gerritConfig));
- assertThat(thrown).hasMessageThat().contains("Can't write to disk cache");
- }
-
- @Test
public void shouldProvideDefinitionRefreshAfterWriteWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getRefreshAfterWrite()).isEqualTo(refreshAfterWrite);
}
@@ -247,15 +208,16 @@
assertThrows(IllegalArgumentException.class, () -> configUnderTest(gerritConfig));
}
- private ChronicleMapCacheConfig configUnderTest(StoredConfig gerritConfig) throws IOException {
+ private ChronicleMapCacheConfig configUnderTest(StoredConfig gerritConfig) {
+ File persistentFile =
+ ChronicleMapCacheFactory.fileName(
+ sitePaths.site_path.resolve(cacheDirectory), cacheName, version);
+ sitePaths
+ .resolve(cacheDirectory)
+ .resolve(String.format("%s_%s.dat", cacheName, version))
+ .toFile();
+
return new ChronicleMapCacheConfig(
- gerritConfig,
- sitePaths,
- cacheName,
- cacheKey,
- definitionDiskLimit,
- expireAfterWrite,
- refreshAfterWrite,
- version);
+ gerritConfig, cacheKey, persistentFile, expireAfterWrite, refreshAfterWrite);
}
}
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 844212a..f8ef939 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
@@ -30,6 +30,7 @@
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
+import java.io.File;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.file.Files;
@@ -53,6 +54,8 @@
private SitePaths sitePaths;
private StoredConfig gerritConfig;
+ private final String cacheDirectory = ".";
+
@Before
public void setUp() throws Exception {
sitePaths = new SitePaths(temporaryFolder.newFolder().toPath());
@@ -62,7 +65,6 @@
new FileBasedConfig(
sitePaths.resolve("etc").resolve("gerrit.config").toFile(), FS.DETECTED);
gerritConfig.load();
- String cacheDirectory = "cache-dir";
gerritConfig.setString("cache", null, "directory", cacheDirectory);
gerritConfig.save();
@@ -507,16 +509,17 @@
throws IOException {
TestPersistentCacheDef cacheDef = new TestPersistentCacheDef(cachedValue);
+ File persistentFile =
+ ChronicleMapCacheFactory.fileName(
+ sitePaths.site_path.resolve(cacheDirectory), cacheDef.name(), version);
+
ChronicleMapCacheConfig config =
new ChronicleMapCacheConfig(
gerritConfig,
- sitePaths,
- cacheDef.name(),
cacheDef.configKey(),
- cacheDef.diskLimit(),
+ persistentFile,
expireAfterWrite != null ? expireAfterWrite : Duration.ZERO,
- refreshAfterWrite != null ? refreshAfterWrite : Duration.ZERO,
- version);
+ refreshAfterWrite != null ? refreshAfterWrite : Duration.ZERO);
return new ChronicleMapCacheImpl<>(
cacheDef, config, withLoader ? cacheDef.loader() : null, metricMaker);
