Fix auto-replay of stored events during config reload Use a proper configuration reload listener to allow ReplicationQueue to be aware of what's going on and perform a proper stop/start sequence instead of relying on a low-level WorkQueue access. The configuration reload behaves now as a soft-restart so that stored events parked on the filesystem won't be missed any more. If the new configuration is invalid and fails to load, keep the current one without retrying until it gets modified again. That avoids the start/stop of the ReplicationQueue continuously. Bug: Issue 10260 Change-Id: Ia34b460221a2e0512a4ffba66d1f08c201402917
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 6ed438a..7374c4a 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
@@ -19,6 +19,7 @@ import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.git.WorkQueue; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.nio.file.Path; @@ -31,17 +32,20 @@ private ReplicationFileBasedConfig currentConfig; private long currentConfigTs; + private long lastFailedConfigTs; private final SitePaths site; - private final WorkQueue workQueue; private final DestinationFactory destinationFactory; private final Path pluginDataDir; + // Use Provider<> instead of injecting the ReplicationQueue because of circular dependency with + // ReplicationConfig + private final Provider<ReplicationQueue> replicationQueue; @Inject public AutoReloadConfigDecorator( SitePaths site, - WorkQueue workQueue, DestinationFactory destinationFactory, + Provider<ReplicationQueue> replicationQueue, @PluginData Path pluginDataDir) throws ConfigInvalidException, IOException { this.site = site; @@ -49,7 +53,7 @@ this.pluginDataDir = pluginDataDir; this.currentConfig = loadConfig(); this.currentConfigTs = getLastModified(currentConfig); - this.workQueue = workQueue; + this.replicationQueue = replicationQueue; } private static long getLastModified(ReplicationFileBasedConfig cfg) { @@ -71,25 +75,27 @@ } private void reloadIfNeeded() { - try { - if (isAutoReload()) { - long lastModified = getLastModified(currentConfig); - if (lastModified > currentConfigTs) { - ReplicationFileBasedConfig newConfig = loadConfig(); - newConfig.startup(workQueue); - int discarded = currentConfig.shutdown(); - - this.currentConfig = newConfig; - this.currentConfigTs = lastModified; + if (isAutoReload()) { + ReplicationQueue queue = replicationQueue.get(); + long lastModified = getLastModified(currentConfig); + try { + if (lastModified > currentConfigTs && lastModified > lastFailedConfigTs) { + queue.stop(); + currentConfig = loadConfig(); + currentConfigTs = lastModified; + lastFailedConfigTs = 0; logger.atInfo().log( - "Configuration reloaded: %d destinations, %d replication events discarded", - currentConfig.getDestinations(FilterType.ALL).size(), discarded); + "Configuration reloaded: %d destinations", + currentConfig.getDestinations(FilterType.ALL).size()); } + } catch (Exception e) { + logger.atSevere().withCause(e).log( + "Cannot reload replication configuration: keeping existing settings"); + lastFailedConfigTs = lastModified; + return; + } finally { + queue.start(); } - } catch (Exception e) { - logger.atSevere().withCause(e).log( - "Cannot reload replication configuration: keeping existing settings"); - return; } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationQueue.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationQueue.java index ae06f43..519d0b4 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationQueue.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationQueue.java
@@ -91,9 +91,11 @@ @Override public void start() { - config.startup(workQueue); - running = true; - firePendingEvents(); + if (!running) { + config.startup(workQueue); + running = true; + firePendingEvents(); + } } @Override