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
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 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 @@
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.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 @@
@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<ConfigResource> base;
+ @Inject private Provider<ConfigResource> baseConfigProvider;
+
+ private final Supplier<ConfigResource> 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 @@
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 @@
@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 @@
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 @@
eventBus,
Providers.of(replicationQueueMock));
return new AutoReloadConfigDecorator(
- "replication",
- workQueueMock,
- newReplicationFileBasedConfig(),
- autoReloadRunnable,
- eventBus);
+ "replication", workQueueMock, autoReloadRunnable, eventBus);
}
}
