From 56b8ffbab5bf619c0b6b5d44f0255fd41b9e1c89 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sun, 7 Apr 2024 10:19:59 -0700 Subject: Load replication configuration only once Reuse the same instance across requests in ReplicationConfigImpl. This avoids reloading the configuration from disk continuously. Previously, the config was retrieved on-demand with the Provider<>.get() paradigm. However, the Provider<>.get() would trigger the creation of a new instance of the Config every time it is invoked, causing the full scan and parsing of the replication configs. On GerritHub.io, the processing of all replication configs (12k files) would take around 5 minutes, causing a catastrophic overload of the CPU and preventing the plugin that use ReplicationConfigImpl (replication and pull-replication) to even start. With regards to the configVersion, the description of the interface has been amended to reflect the expected behaviour to report the *latest actual* version on the storage, regardless of the in-memory copy. The AutoReloadConfigDectorator relies on the configVersion to reflect the storage version for triggering a configuration reload by forcibly re-creating the config resources and therefore triggering the reload and parsing of the replication configs from the underlying storage. AutoReloadConfigDecorator should not reload the configuration again in his constructor because the AutoReloadRunnable has already loaded the initial state and therefore can reuse that one as valid starting point. Change-Id: I6f9c3a35d1ecbfa87f3daeeb464face0c6b6400e --- .../gerrit/plugins/replication/AutoReloadConfigDecorator.java | 3 +-- .../gerrit/plugins/replication/AutoReloadRunnable.java | 4 ++++ .../gerrit/plugins/replication/MergedConfigResource.java | 9 +++++++-- .../gerrit/plugins/replication/ReplicationConfigImpl.java | 7 ++++--- .../gerrit/plugins/replication/api/ConfigResource.java | 5 ++--- .../plugins/replication/AutoReloadConfigDecoratorTest.java | 6 +----- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java index 9c658e0..2c049fc 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java @@ -43,10 +43,9 @@ public class AutoReloadConfigDecorator implements ReplicationConfig, LifecycleLi public AutoReloadConfigDecorator( @PluginName String pluginName, WorkQueue workQueue, - ReplicationConfigImpl replicationConfig, AutoReloadRunnable reloadRunner, EventBus eventBus) { - this.currentConfig = replicationConfig; + this.currentConfig = reloadRunner.getCurrentReplicationConfig(); this.autoReloadExecutor = workQueue.createQueue(1, pluginName + "_auto-reload-config"); this.reloadRunner = reloadRunner; eventBus.register(this); diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java index e9198b2..a752a35 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java @@ -61,6 +61,10 @@ public class AutoReloadRunnable implements Runnable { reload(); } + public ReplicationConfig getCurrentReplicationConfig() { + return loadedConfig; + } + synchronized void reload() { String pendingConfigVersion = loadedConfig.getVersion(); try { diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java index 5e8100b..43d90a5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java @@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.replication; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Suppliers; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.UsedAt; @@ -25,6 +26,7 @@ import com.google.inject.Provider; import com.google.inject.util.Providers; import com.googlesource.gerrit.plugins.replication.api.ConfigResource; import com.googlesource.gerrit.plugins.replication.api.ReplicationConfigOverrides; +import java.util.function.Supplier; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @@ -33,13 +35,16 @@ public class MergedConfigResource { @UsedAt(Project.PLUGIN_PULL_REPLICATION) public static MergedConfigResource withBaseOnly(ConfigResource base) { MergedConfigResource mergedConfigResource = new MergedConfigResource(); - mergedConfigResource.base = Providers.of(base); + mergedConfigResource.baseConfigProvider = Providers.of(base); return mergedConfigResource; } private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - @Inject private Provider base; + @Inject private Provider baseConfigProvider; + + private final Supplier base = + Suppliers.memoize(() -> this.baseConfigProvider.get()); @Inject(optional = true) @Nullable diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java index 854ca6a..1223db0 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java @@ -30,21 +30,22 @@ public class ReplicationConfigImpl implements ReplicationConfig { private static final int DEFAULT_SSH_CONNECTION_TIMEOUT_MS = 2 * 60 * 1000; // 2 minutes private final SitePaths site; + private final MergedConfigResource configResource; private boolean replicateAllOnPluginStart; private boolean defaultForceUpdate; private int maxRefsToLog; private final int maxRefsToShow; private int sshCommandTimeout; private int sshConnectionTimeout; - private final MergedConfigResource configResource; private final Path pluginDataDir; + private final Config config; @Inject public ReplicationConfigImpl( MergedConfigResource configResource, SitePaths site, @PluginData Path pluginDataDir) { this.site = site; + config = configResource.getConfig(); this.configResource = configResource; - Config config = configResource.getConfig(); this.replicateAllOnPluginStart = config.getBoolean("gerrit", "replicateOnStartup", false); this.defaultForceUpdate = config.getBoolean("gerrit", "defaultForceUpdate", false); this.maxRefsToLog = config.getInt("gerrit", "maxRefsToLog", 0); @@ -120,7 +121,7 @@ public class ReplicationConfigImpl implements ReplicationConfig { @Override public Config getConfig() { - return configResource.getConfig(); + return config; } @Override diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java index 8babd01..43733bc 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java @@ -37,10 +37,9 @@ public interface ConfigResource { Config getConfig(); /** - * Current logical version string of the current configuration loaded in memory, depending on the - * actual implementation of the configuration on the persistent storage. + * Current logical version string of the current configuration on the persistent storage. * - * @return current logical version number. + * @return latest logical version number on the persistent storage */ String getVersion(); } diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java index 6facf25..021707a 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java @@ -264,10 +264,6 @@ public class AutoReloadConfigDecoratorTest extends AbstractConfigTest { eventBus, Providers.of(replicationQueueMock)); return new AutoReloadConfigDecorator( - "replication", - workQueueMock, - newReplicationFileBasedConfig(), - autoReloadRunnable, - eventBus); + "replication", workQueueMock, autoReloadRunnable, eventBus); } } -- cgit v1.2.3