Improve FanoutConfigurationResource loading performance
When using replication remotes as individual files under
$GERRIT_SITE/etc/replication the time requested to load the remotes,
parsing them and merging them together can be very significant,
especially when having a lot of remote configurations.
On GerritHub.io with 12k remotes configured, the time to load,
parse and merge them in a unique FileBasedConfig object reaches
several minutes.
The main issue found during profiling is the manual merging of
Config through one call per setting using Config.setStringList()
rather than merging them first and then parsing them as a whole.
Refactor the overall parsing logic by splitting the process in
three parts:
- load of all replication remotes a list of String
- expanding them as individual strings and adding the remote names
- merging them together with the base config using the JGit's
default mechanism
From an external perspective, the API work exactly as before, including
the config validation process; from a performance perspective, the
loading of 12k remotes is reduced by 20x times, taking under 1 minute.
Change-Id: I2880428b881034e6253d07ab0ba368c0a97eecc0
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java
index 5d308a5..1ecf242 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java
@@ -17,7 +17,6 @@
import static com.google.common.io.Files.getNameWithoutExtension;
import static java.nio.charset.StandardCharsets.UTF_8;
-import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.Hasher;
import com.google.common.hash.Hashing;
@@ -28,9 +27,10 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
-import java.util.Optional;
import java.util.Set;
+import java.util.function.Function;
import java.util.function.Predicate;
+import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.internal.storage.file.FileSnapshot;
@@ -43,6 +43,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Path remoteConfigsDirPath;
+ private final Config fanoutConfig;
@Inject
FanoutConfigResource(SitePaths site) throws IOException, ConfigInvalidException {
@@ -51,16 +52,35 @@
removeRemotes(config);
try (Stream<Path> files = Files.list(remoteConfigsDirPath)) {
- files
- .filter(Files::isRegularFile)
- .filter(FanoutConfigResource::isConfig)
- .map(FanoutConfigResource::loadConfig)
- .filter(Optional::isPresent)
- .map(Optional::get)
- .filter(FanoutConfigResource::isValid)
- .forEach(cfg -> addRemoteConfig(cfg, config));
- } catch (IllegalStateException e) {
- throw new ConfigInvalidException(e.getMessage());
+ Stream<String> mergedConfigs =
+ files
+ .filter(Files::isRegularFile)
+ .filter(FanoutConfigResource::isConfig)
+ .flatMap(path -> getConfigLines(path, line -> addRemoteSubsection(line, path)));
+
+ fanoutConfig = new Config(config);
+ fanoutConfig.fromText(mergedConfigs.collect(Collectors.joining("\n")));
+ }
+ }
+
+ private static String addRemoteSubsection(String line, Path path) {
+ return line.contains("[remote]")
+ ? line.replace(
+ "[remote]", "[remote \"" + getNameWithoutExtension(path.toFile().getName()) + "\"]")
+ : line;
+ }
+
+ private static Stream<String> getConfigLines(
+ Path path, Function<String, String> configLineMapping) {
+ try {
+ List<String> configLines = Files.readAllLines(path);
+ if (!isValid(path, configLines)) {
+ return Stream.empty();
+ }
+ return configLines.stream().map(configLineMapping);
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Unable to access replication config %s", path);
+ return Stream.empty();
}
}
@@ -101,17 +121,6 @@
}
}
- private static void addRemoteConfig(FileBasedConfig source, Config destination) {
- String remoteName = getNameWithoutExtension(source.getFile().getName());
- for (String name : source.getNames("remote")) {
- destination.setStringList(
- "remote",
- remoteName,
- name,
- Lists.newArrayList(source.getStringList("remote", null, name)));
- }
- }
-
private static Config filterOutRemotes(Config config) {
Config filteredConfig = new Config();
Set<String> sections = config.getSections();
@@ -146,31 +155,33 @@
return filteredConfig;
}
- private static boolean isValid(Config cfg) {
- if (cfg.getSections().size() != 1 || !cfg.getSections().contains("remote")) {
- logger.atSevere().log(
- "Remote replication configuration file %s must contain only one remote section.", cfg);
- return false;
- }
- if (cfg.getSubsections("remote").size() > 0) {
- logger.atSevere().log(
- "Remote replication configuration file %s cannot contain remote subsections.", cfg);
- return false;
+ private static boolean isValid(Path path, List<String> remoteConfigLines) {
+ int numRemoteSections = 0;
+ int numRemoteSectionsWithName = 0;
+ boolean valid = true;
+ for (String configLine : remoteConfigLines) {
+ if (configLine.contains("[remote]")) {
+ numRemoteSections++;
+ }
+
+ if (configLine.contains("[remote \"")) {
+ numRemoteSectionsWithName++;
+ }
}
- return true;
- }
-
- private static Optional<FileBasedConfig> loadConfig(Path path) {
- FileBasedConfig cfg = new FileBasedConfig(path.toFile(), FS.DETECTED);
- try {
- cfg.load();
- } catch (IOException | ConfigInvalidException e) {
- logger.atSevere().withCause(e).log(
- "Cannot load remote replication configuration file %s.", path);
- return Optional.empty();
+ if (numRemoteSectionsWithName > 0) {
+ logger.atSevere().log(
+ "Remote replication configuration file %s cannot contain remote subsections.", path);
+ valid = false;
}
- return Optional.of(cfg);
+
+ if (numRemoteSections != 1) {
+ logger.atSevere().log(
+ "Remote replication configuration file %s must contain only one remote section.", path);
+ valid = false;
+ }
+
+ return valid;
}
private static boolean isConfig(Path p) {
@@ -201,4 +212,9 @@
return parentVersion;
}
}
+
+ @Override
+ public Config getConfig() {
+ return fanoutConfig;
+ }
}
