Merge pull request #25722 from vespa-engine/hmusum/file-registry-refactoring

Hmusum/file registry refactoring

5 files changed, 33 insertions, 27 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java
index 9d915650c73..48f931db053 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java
@@ -22,6 +22,8 @@ import java.util.Optional;
 import java.util.regex.Pattern;
 
 /**
+ * File registry for one application package
+ *
  * @author Tony Vaagenes
  */
 public class FileDBRegistry implements FileRegistry {
@@ -105,16 +107,14 @@ public class FileDBRegistry implements FileRegistry {
     }
 
     @Override
-    public FileReference addBlob(String blobName, ByteBuffer blob) {
+    public synchronized FileReference addBlob(String blobName, ByteBuffer blob) {
         String relativePath = blobToRelativeFile(blobName);
-        synchronized (this) {
-            Optional<FileReference> cachedReference = Optional.ofNullable(fileReferenceCache.get(blobName));
-            return cachedReference.orElseGet(() -> {
-                FileReference newRef = manager.addBlob(blob, Path.fromString(relativePath));
-                fileReferenceCache.put(blobName, newRef);
-                return newRef;
-            });
-        }
+        Optional<FileReference> cachedReference = Optional.ofNullable(fileReferenceCache.get(blobName));
+        return cachedReference.orElseGet(() -> {
+            FileReference newRef = manager.addBlob(blob, Path.fromString(relativePath));
+            fileReferenceCache.put(blobName, newRef);
+            return newRef;
+        });
     }
 
     @Override
@@ -126,6 +126,7 @@ public class FileDBRegistry implements FileRegistry {
         return entries;
     }
 
+    // Used for testing only
     synchronized Map<String, FileReference> getMap() {
         return ImmutableMap.copyOf(fileReferenceCache);
     }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java
index a999498577c..85b30f4d303 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java
@@ -30,6 +30,10 @@ import java.util.logging.Logger;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
 
+/**
+ * Global file directory, holding files for file distribution for all deployed applications.
+ *
+ */
 public class FileDirectory extends AbstractComponent {
 
     private static final Logger log = Logger.getLogger(FileDirectory.class.getName());
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ApplicationPackageMaintainer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ApplicationPackageMaintainer.java
index df92dbac70f..346a462fe3e 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ApplicationPackageMaintainer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ApplicationPackageMaintainer.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.maintenance;
 
-import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.config.FileReference;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.subscription.ConfigSourceSet;
@@ -28,13 +27,13 @@ import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 import static com.yahoo.vespa.config.server.filedistribution.FileDistributionUtil.fileReferenceExistsOnDisk;
-import static com.yahoo.vespa.config.server.filedistribution.FileDistributionUtil.getOtherConfigServersInCluster;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType;
 
 /**
  * Verifies that all active sessions has an application package on local disk.
  * If not, the package is downloaded with file distribution. This can happen e.g.
- * if a config server is down when the application is deployed.
+ * if a config server is down when the application is deployed. This maintainer should only be run
+ * if there is more than 1 config server
  *
  * @author gjoranv
  */
@@ -44,19 +43,18 @@ public class ApplicationPackageMaintainer extends ConfigServerMaintainer {
 
     private final ApplicationRepository applicationRepository;
     private final File downloadDirectory;
-    private final ConfigserverConfig configserverConfig;
     private final Supervisor supervisor = new Supervisor(new Transport("filedistribution-pool")).setDropEmptyBuffers(true);
     private final FileDownloader fileDownloader;
 
     ApplicationPackageMaintainer(ApplicationRepository applicationRepository,
                                  Curator curator,
                                  Duration interval,
-                                 FlagSource flagSource) {
+                                 FlagSource flagSource,
+                                 List<String> otherConfigServersInCluster) {
         super(applicationRepository, curator, flagSource, applicationRepository.clock(), interval, false);
         this.applicationRepository = applicationRepository;
-        this.configserverConfig = applicationRepository.configserverConfig();
-        this.downloadDirectory = new File(Defaults.getDefaults().underVespaHome(configserverConfig.fileReferencesDir()));
-        this.fileDownloader = createFileDownloader(configserverConfig,
+        this.downloadDirectory = new File(Defaults.getDefaults().underVespaHome(applicationRepository.configserverConfig().fileReferencesDir()));
+        this.fileDownloader = createFileDownloader(otherConfigServersInCluster,
                                                    downloadDirectory,
                                                    supervisor,
                                                    Flags.FILE_DISTRIBUTION_ACCEPTED_COMPRESSION_TYPES.bindTo(flagSource).value());
@@ -64,8 +62,6 @@ public class ApplicationPackageMaintainer extends ConfigServerMaintainer {
 
     @Override
     protected double maintain() {
-        if (getOtherConfigServersInCluster(configserverConfig).isEmpty()) return 1.0; // Nothing to do
-
         int attempts = 0;
         int failures = 0;
 
@@ -102,16 +98,12 @@ public class ApplicationPackageMaintainer extends ConfigServerMaintainer {
         return asSuccessFactor(attempts, failures);
     }
 
-    private static FileDownloader createFileDownloader(ConfigserverConfig configserverConfig,
+    private static FileDownloader createFileDownloader(List<String> otherConfigServersInCluster,
                                                        File downloadDirectory,
                                                        Supervisor supervisor,
                                                        List<String> flagValues) {
-        List<String> otherConfigServersInCluster = getOtherConfigServersInCluster(configserverConfig);
         ConfigSourceSet configSourceSet = new ConfigSourceSet(otherConfigServersInCluster);
-
-        ConnectionPool connectionPool = (otherConfigServersInCluster.isEmpty())
-                ? FileDownloader.emptyConnectionPool()
-                : new FileDistributionConnectionPool(configSourceSet, supervisor);
+        ConnectionPool connectionPool = new FileDistributionConnectionPool(configSourceSet, supervisor);
         Set<CompressionType> acceptedCompressionTypes = flagValues.stream()
                                                                   .map(CompressionType::valueOf)
                                                                   .collect(Collectors.toSet());
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintenance.java b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintenance.java
index 7dda3d4e462..8d5e1fe9dea 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintenance.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintenance.java
@@ -12,6 +12,8 @@ import java.time.Duration;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
 
+import static com.yahoo.vespa.config.server.filedistribution.FileDistributionUtil.getOtherConfigServersInCluster;
+
 /**
  * Maintenance jobs of the config server.
  * Each maintenance job is a singleton instance of its implementing class, created and owned by this,
@@ -39,7 +41,10 @@ public class ConfigServerMaintenance {
     }
 
     public void startBeforeBootstrap() {
-        maintainers.add(new ApplicationPackageMaintainer(applicationRepository, curator, Duration.ofSeconds(30), flagSource));
+        List<String> otherConfigServersInCluster = getOtherConfigServersInCluster(applicationRepository.configserverConfig());
+        if ( ! otherConfigServersInCluster.isEmpty())
+            maintainers.add(new ApplicationPackageMaintainer(applicationRepository, curator, Duration.ofSeconds(30),
+                                                             flagSource, otherConfigServersInCluster));
         maintainers.add(new TenantsMaintainer(applicationRepository, curator, flagSource, interval, Clock.systemUTC()));
     }
 
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java
index fd2b7fe8a77..2351706659a 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java
@@ -214,6 +214,9 @@ public class ConfigServerBootstrapTest {
     }
 
     private static ConfigserverConfig createConfigserverConfig(TemporaryFolder temporaryFolder, boolean hosted) throws IOException {
+        var servers = List.of(new ConfigserverConfig.Zookeeperserver.Builder().hostname("foo").port(1),
+                              new ConfigserverConfig.Zookeeperserver.Builder().hostname("bar").port(1),
+                              new ConfigserverConfig.Zookeeperserver.Builder().hostname("baz").port(1));
         return new ConfigserverConfig(new ConfigserverConfig.Builder()
                                               .configServerDBDir(temporaryFolder.newFolder("serverdb").getAbsolutePath())
                                               .configDefinitionsDir(temporaryFolder.newFolder("configdefinitions").getAbsolutePath())
@@ -221,7 +224,8 @@ public class ConfigServerBootstrapTest {
                                               .hostedVespa(hosted)
                                               .multitenant(hosted)
                                               .maxDurationOfBootstrap(0) /* seconds, 0 => it will not retry deployment if bootstrap fails */
-                                              .sleepTimeWhenRedeployingFails(0)); /* seconds */
+                                              .sleepTimeWhenRedeployingFails(0) /* seconds */
+                                              .zookeeperserver(servers));
     }
 
     private List<Host> createHosts(String vespaVersion) {