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

Hmusum/file registry refactoring

10 files changed, 51 insertions, 111 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/FileRegistry.java b/config-model-api/src/main/java/com/yahoo/config/application/api/FileRegistry.java
index c2cde72449b..69836ab06c3 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/FileRegistry.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/FileRegistry.java
@@ -5,6 +5,8 @@ import com.yahoo.config.FileReference;
 
 import java.nio.ByteBuffer;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * @author Tony Vaagenes
@@ -16,6 +18,11 @@ public interface FileRegistry {
     FileReference addBlob(String name, ByteBuffer blob);
     default FileReference addApplicationPackage() { return addFile(""); }
     List<Entry> export();
+    default Set<FileReference> asSet() {
+        return export().stream()
+                       .map(e -> e.reference)
+                       .collect(Collectors.toSet());
+    }
 
     class Entry {
 
diff --git a/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java b/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java
index 74a5e92d2ba..5f346f366de 100644
--- a/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java
+++ b/config-model/src/main/java/com/yahoo/config/model/test/MockRoot.java
@@ -17,9 +17,7 @@ import com.yahoo.vespa.model.HostSystem;
 import com.yahoo.vespa.model.admin.Admin;
 import com.yahoo.vespa.model.builder.xml.dom.DomAdminV2Builder;
 import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProducer;
-import com.yahoo.vespa.model.filedistribution.FileReferencesRepository;
 import org.w3c.dom.Document;
-
 import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -35,12 +33,9 @@ import java.util.Set;
 // TODO: mockRoot instances can probably be replaced by VespaModel.createIncomplete
 public class MockRoot extends AbstractConfigProducerRoot {
 
-    private static final long serialVersionUID = 1L;
-
     private final HostSystem hostSystem;
 
     private final DeployState deployState;
-    private final FileReferencesRepository fileReferencesRepository;
     private Admin admin;
 
     public MockRoot() {
@@ -63,7 +58,6 @@ public class MockRoot extends AbstractConfigProducerRoot {
         super(rootConfigId);
         hostSystem = new HostSystem(this, "hostsystem", deployState.getProvisioner(), deployState.getDeployLogger(), deployState.isHosted());
         this.deployState = deployState;
-        fileReferencesRepository = new FileReferencesRepository(deployState.getFileRegistry());
     }
 
     public FileDistributionConfigProducer getFileDistributionConfigProducer() {
@@ -89,7 +83,6 @@ public class MockRoot extends AbstractConfigProducerRoot {
         return builder;
     }
 
-    @SuppressWarnings("unchecked")
     public <T extends ConfigInstance> T getConfig(Class<T> configClass, String configId) {
         try {
             ConfigInstance.Builder builder = getConfig(getBuilder(configClass).getDeclaredConstructor().newInstance(), configId);
@@ -116,8 +109,6 @@ public class MockRoot extends AbstractConfigProducerRoot {
         return deployState;
     }
 
-    public FileReferencesRepository fileReferencesRepository() { return fileReferencesRepository; }
-
     public HostSystem hostSystem() { return hostSystem; }
 
     public void addDescendant(String configId, AbstractConfigProducer<?> descendant) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
index a13eebf0042..69aacb75fb9 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
@@ -11,13 +11,17 @@ import com.yahoo.config.FileReference;
 import com.yahoo.config.application.api.ApplicationFile;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.config.application.api.FileRegistry;
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.application.api.ValidationOverrides;
 import com.yahoo.config.model.ApplicationConfigProducerRoot;
 import com.yahoo.config.model.ConfigModelRegistry;
 import com.yahoo.config.model.ConfigModelRepo;
 import com.yahoo.config.model.NullConfigModelRegistry;
-import com.yahoo.config.model.api.*;
+import com.yahoo.config.model.api.ApplicationClusterInfo;
+import com.yahoo.config.model.api.HostInfo;
+import com.yahoo.config.model.api.Model;
+import com.yahoo.config.model.api.Provisioned;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.config.model.producer.AbstractConfigProducerRoot;
@@ -47,7 +51,6 @@ import com.yahoo.vespa.model.container.search.QueryProfiles;
 import com.yahoo.vespa.model.content.Content;
 import com.yahoo.vespa.model.content.cluster.ContentCluster;
 import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProducer;
-import com.yahoo.vespa.model.filedistribution.FileReferencesRepository;
 import com.yahoo.vespa.model.ml.ConvertedModel;
 import com.yahoo.vespa.model.ml.ModelName;
 import com.yahoo.vespa.model.ml.OnnxModelInfo;
@@ -56,7 +59,6 @@ import com.yahoo.vespa.model.search.DocumentDatabase;
 import com.yahoo.vespa.model.search.SearchCluster;
 import com.yahoo.vespa.model.utils.internal.ReflectionUtil;
 import org.xml.sax.SAXException;
-
 import java.io.IOException;
 import java.lang.reflect.Constructor;
 import java.time.Instant;
@@ -114,7 +116,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Mode
     /** The validation overrides of this. This is never null. */
     private final ValidationOverrides validationOverrides;
 
-    private final FileReferencesRepository fileReferencesRepository;
+    private final FileRegistry fileRegistry;
 
     private final Provisioned provisioned;
 
@@ -150,12 +152,11 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Mode
         this(configModelRegistry, deployState, true);
     }
 
-    private VespaModel(ConfigModelRegistry configModelRegistry, DeployState deployState, boolean complete)
-            throws IOException, SAXException {
+    private VespaModel(ConfigModelRegistry configModelRegistry, DeployState deployState, boolean complete) throws IOException {
         super("vespamodel");
         version = deployState.getVespaVersion();
         wantedNodeVersion = deployState.getWantedNodeVespaVersion();
-        fileReferencesRepository = new FileReferencesRepository(deployState.getFileRegistry());
+        fileRegistry = deployState.getFileRegistry();
         validationOverrides = deployState.validationOverrides();
         applicationPackage = deployState.getApplicationPackage();
         provisioned = deployState.provisioned();
@@ -235,7 +236,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Mode
     public ApplicationPackage applicationPackage() { return applicationPackage; }
 
     /** Creates a mutable model with no services instantiated */
-    public static VespaModel createIncomplete(DeployState deployState) throws IOException, SAXException {
+    public static VespaModel createIncomplete(DeployState deployState) throws IOException {
         return new VespaModel(new NullConfigModelRegistry(), deployState, false);
     }
 
@@ -353,7 +354,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Mode
                            .collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
-    public Set<FileReference> fileReferences() { return fileReferencesRepository.allFileReferences(); }
+    public Set<FileReference> fileReferences() { return fileRegistry.asSet(); }
 
     /** Returns this models Vespa instance */
     public ApplicationConfigProducerRoot getVespa() { return root; }
@@ -477,7 +478,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Mode
      * Resolves the given config key into a correctly typed ConfigBuilder
      * and fills in the config from this model.
      *
-     * @return A new config builder with config from this model filled in,.
+     * @return A new config builder with config from this model filled in
      */
     private ConfigInstance.Builder resolveToBuilder(ConfigKey<?> key) {
         ConfigInstance.Builder builder = createBuilder(new ConfigDefinitionKey(key));
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileReferencesRepository.java b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileReferencesRepository.java
deleted file mode 100644
index 3962aa3d612..00000000000
--- a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileReferencesRepository.java
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.model.filedistribution;
-
-import com.yahoo.config.FileReference;
-import com.yahoo.config.application.api.FileRegistry;
-
-import java.util.LinkedHashSet;
-import java.util.Set;
-import java.util.stream.Collectors;
-
-/**
- * Keeps track of what files to send with file distribution
- *
- * @author Tony Vaagenes
- * @author hmusum
- */
-public class FileReferencesRepository {
-
-    private final FileRegistry fileRegistry;
-    public FileReferencesRepository(FileRegistry fileRegistry) {
-        this.fileRegistry = fileRegistry;
-    }
-
-    public Set<FileReference> allFileReferences() {
-        return fileRegistry.export()
-                           .stream()
-                           .map(e -> e.reference)
-                           .collect(Collectors.toCollection(() -> new LinkedHashSet<>()));
-    }
-
-}
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/FileReferencesRepositoryTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/FileReferencesRepositoryTestCase.java
deleted file mode 100644
index 628e87e24c5..00000000000
--- a/config-model/src/test/java/com/yahoo/vespa/model/filedistribution/FileReferencesRepositoryTestCase.java
+++ /dev/null
@@ -1,34 +0,0 @@
-// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.model.filedistribution;
-
-import com.yahoo.config.FileReference;
-import com.yahoo.config.application.api.FileRegistry;
-import com.yahoo.config.model.application.provider.MockFileRegistry;
-import org.junit.jupiter.api.Test;
-
-import java.util.Set;
-
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-
-/**
- * @author bratseth
- */
-public class FileReferencesRepositoryTestCase {
-
-    @Test
-    void fileDistributor() {
-        FileRegistry fileRegistry = new MockFileRegistry();
-        FileReferencesRepository fileReferencesRepository = new FileReferencesRepository(fileRegistry);
-
-        String file1 = "component/path1";
-        String file2 = "component/path2";
-        FileReference ref1 = fileRegistry.addFile(file1);
-        FileReference ref2 = fileRegistry.addFile(file2);
-
-        assertEquals(Set.of(ref1, ref2), fileReferencesRepository.allFileReferences());
-        assertNotNull(ref1);
-        assertNotNull(ref2);
-    }
-
-}
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) {