Skip unnecessary auto-adjustments of caches
Auto-adjusting the caches is an expensive operation
that could also occupy a considerable amount of extra
disk-space.
Re-building a cache that is already tuned would waste
precious resources and would not communicate effectively
whether a config change is needed or not.
Show only the config changes needed and rebuild only
the caches that need adjusting, leaving everything else
unchanged.
Bug: Issue 14876
Change-Id: Ic4d703d04410924c855ad2b96f8f635994d968a8
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
index a9f8bb7..c85dc43 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
@@ -38,7 +38,7 @@
public class AutoAdjustCaches extends SshCommand {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
protected static final String CONFIG_HEADER = "__CONFIG__";
- protected static final String TUNED_INFIX = "tuned";
+ protected static final String TUNED_INFIX = "_tuned_";
private final DynamicMap<Cache<?, ?>> cacheMap;
private final ChronicleMapCacheConfig.Factory configFactory;
@@ -84,6 +84,14 @@
long averageKeySize = avgSizes.getKey();
long averageValueSize = avgSizes.getValue();
+
+ ChronicleMapCacheConfig currCacheConfig = currCache.getConfig();
+
+ if (currCacheConfig.getAverageKeySize() == averageKeySize
+ && currCacheConfig.getAverageValueSize() == averageValueSize) {
+ return;
+ }
+
ChronicleMapCacheConfig newChronicleMapCacheConfig =
makeChronicleMapConfig(currCache.getConfig(), averageKeySize, averageValueSize);
@@ -129,9 +137,15 @@
});
stdout.println();
- stdout.println("****************************");
- stdout.println("** Chronicle-map template **");
- stdout.println("****************************");
+ stdout.println("**********************************");
+
+ if (outputChronicleMapConfig.getSections().isEmpty()) {
+ stdout.println("All exsting caches are already tuned: no changes needed.");
+ return;
+ }
+
+ stdout.println("** Chronicle-map config changes **");
+ stdout.println("**********************************");
stdout.println();
stdout.println(CONFIG_HEADER);
stdout.println(outputChronicleMapConfig.toText());
@@ -186,7 +200,7 @@
private File resolveNewFile(String currentFileName) {
String newFileName =
String.format(
- "%s_%s_%s.%s",
+ "%s%s%s.%s",
FilenameUtils.getBaseName(currentFileName),
TUNED_INFIX,
System.currentTimeMillis(),
diff --git a/src/main/resources/Documentation/tuning.md b/src/main/resources/Documentation/tuning.md
index a27cb7c..18f7eca 100644
--- a/src/main/resources/Documentation/tuning.md
+++ b/src/main/resources/Documentation/tuning.md
@@ -156,15 +156,15 @@
Calculate the average key and value size, but do not migrate current cache
data into new files
-For each chronicle-map cache (i.e. `foo_1.dat` file) in the `cache` directory, a
-new one will be created (i.e. `foo_1_tuned_<timestamp>.dat`).
+For each chronicle-map cache that needs tuning (i.e. `foo_1.dat` file) in
+the `cache` directory, a new one will be created (i.e. `foo_1_tuned_<timestamp>.dat`).
The new cache will have these characteristics:
- Will have the same entries as the original cache.
- Will be configured with the *actual* average key size and values calculated by
looking at the content of the original cache.
-An output will also be generated with the new configuration that should be put
-into `gerrit.config`, should you decide to use the new caches.
+An output will also be generated with the configuration changes that should
+be included into `gerrit.config`, should you decide to use the new caches.
An example of the output is the following:
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
index bc6a592..7d63239 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
@@ -20,15 +20,24 @@
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.Defaults.maxBloatFactorFor;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.Defaults.maxEntriesFor;
+import com.google.common.base.Joiner;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.server.ModuleImpl;
+import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
+import com.google.inject.name.Named;
import java.io.File;
+import java.nio.file.Path;
+import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
@@ -50,15 +59,44 @@
private static final String DIFF_SUMMARY = "diff_summary";
private static final String ACCOUNTS = "accounts";
private static final String PERSISTED_PROJECTS = "persisted_projects";
+ private static final String TEST_CACHE_NAME = "test_cache";
+ private static final int TEST_CACHE_VERSION = 1;
+ private static final String TEST_CACHE_FILENAME_TUNED =
+ TEST_CACHE_NAME + "_" + TEST_CACHE_VERSION + AutoAdjustCaches.TUNED_INFIX;
+ private static final String TEST_CACHE_KEY_100_CHARS = new String(new char[100]);
private static final ImmutableList<String> EXPECTED_CACHES =
ImmutableList.of(MERGEABILITY, DIFF, DIFF_SUMMARY, ACCOUNTS, PERSISTED_PROJECTS);
@Inject private SitePaths sitePaths;
+ @Inject
+ @Named(TEST_CACHE_NAME)
+ LoadingCache<String, String> testCache;
+
+ @ModuleImpl(name = CacheModule.PERSISTENT_MODULE)
+ public static class TestPersistentCacheModule extends CacheModule {
+
+ @Override
+ protected void configure() {
+ persist(TEST_CACHE_NAME, String.class, String.class)
+ .loader(TestCacheLoader.class)
+ .version(TEST_CACHE_VERSION);
+ install(new ChronicleMapCacheModule());
+ }
+ }
+
+ public static class TestCacheLoader extends CacheLoader<String, String> {
+
+ @Override
+ public String load(String key) throws Exception {
+ return key;
+ }
+ }
+
@Override
public com.google.inject.Module createModule() {
- return new ChronicleMapCacheModule();
+ return new TestPersistentCacheModule();
}
@Test
@@ -100,6 +138,31 @@
}
@Test
+ @GerritConfig(name = "cache.test_cache.avgKeySize", value = "207")
+ @GerritConfig(name = "cache.test_cache.avgValueSize", value = "207")
+ public void shouldNotRecreateTestCacheFileWhenAlreadyTuned() throws Exception {
+ testCache.get(TEST_CACHE_KEY_100_CHARS);
+
+ String tuneResult = adminSshSession.exec(CMD);
+ adminSshSession.assertSuccess();
+
+ assertThat(configResult(tuneResult).getSubsections("cache")).doesNotContain(TEST_CACHE_NAME);
+ assertThat(Joiner.on('\n').join(listTunedFileNames()))
+ .doesNotContain(TEST_CACHE_FILENAME_TUNED);
+ }
+
+ @Test
+ public void shouldCreateTestCacheTuned() throws Exception {
+ testCache.get(TEST_CACHE_KEY_100_CHARS);
+
+ String tuneResult = adminSshSession.exec(CMD);
+ adminSshSession.assertSuccess();
+
+ assertThat(configResult(tuneResult).getSubsections("cache")).contains(TEST_CACHE_NAME);
+ assertThat(Joiner.on('\n').join(listTunedFileNames())).contains(TEST_CACHE_FILENAME_TUNED);
+ }
+
+ @Test
public void shouldDenyAccessToCreateNewCacheFiles() throws Exception {
userSshSession.exec(CMD);
userSshSession.assertFailure("not permitted");
@@ -110,4 +173,13 @@
configResult.fromText((result.split(CONFIG_HEADER))[1]);
return configResult;
}
+
+ private List<String> listTunedFileNames() {
+ Path cachePath = sitePaths.resolve(cfg.getString("cache", null, "directory"));
+ return Stream.of(Objects.requireNonNull(cachePath.toFile().listFiles()))
+ .filter(file -> !file.isDirectory())
+ .map(File::getName)
+ .filter(n -> n.contains(TUNED_INFIX))
+ .collect(Collectors.toList());
+ }
}
