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()); + } }