Merge pull request #28066 from vespa-engine/hmusum/return-optional

File serving cleanup

5 files changed, 69 insertions, 61 deletions

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 da18c4e4fcc..6fe133958f5 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
@@ -24,12 +24,14 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.time.Clock;
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
+import static java.util.logging.Level.INFO;
 
 /**
  * Global file directory, holding files for file distribution for all deployed applications.
@@ -40,7 +42,6 @@ public class FileDirectory extends AbstractComponent {
     private static final Logger log = Logger.getLogger(FileDirectory.class.getName());
 
     private final Locks<FileReference> locks = new Locks<>(1, TimeUnit.MINUTES);
-
     private final File root;
 
     @Inject
@@ -67,7 +68,7 @@ public class FileDirectory extends AbstractComponent {
         }
     }
 
-    static private class Filter implements FilenameFilter {
+    private static class Filter implements FilenameFilter {
         @Override
         public boolean accept(File dir, String name) {
             return !".".equals(name) && !"..".equals(name) ;
@@ -78,17 +79,23 @@ public class FileDirectory extends AbstractComponent {
         return root.getAbsolutePath() + "/" + ref.value();
     }
 
-    public File getFile(FileReference reference) {
+    public Optional<File> getFile(FileReference reference) {
         ensureRootExist();
         File dir = new File(getPath(reference));
-        if (!dir.exists())
-            throw new IllegalArgumentException("File reference '" + reference.value() + "' with absolute path '" + dir.getAbsolutePath() + "' does not exist.");
-        if (!dir.isDirectory())
-            throw new IllegalArgumentException("File reference '" + reference.value() + "' with absolute path '" + dir.getAbsolutePath() + "' is not a directory.");
-        File [] files = dir.listFiles(new Filter());
-        if (files == null || files.length == 0)
-            throw new IllegalArgumentException("File reference '" + reference.value() + "' with absolute path '" + dir.getAbsolutePath() + " does not contain any files");
-        return files[0];
+        if (!dir.exists()) {
+            log.log(INFO, "File reference '" + reference.value() + "' ('" + dir.getAbsolutePath() + "') does not exist.");
+            return Optional.empty();
+        }
+        if (!dir.isDirectory()) {
+            log.log(INFO, "File reference '" + reference.value() + "' ('" + dir.getAbsolutePath() + ")' is not a directory.");
+            return Optional.empty();
+        }
+        File[] files = dir.listFiles(new Filter());
+        if (files == null || files.length == 0) {
+            log.log(INFO, "File reference '" + reference.value() + "' ('" + dir.getAbsolutePath() + "') does not contain any files");
+            return Optional.empty();
+        }
+        return Optional.of(files[0]);
     }
 
     public File getRoot() { return root; }
@@ -136,7 +143,7 @@ public class FileDirectory extends AbstractComponent {
     private void deleteDirRecursively(File dir) {
         log.log(Level.FINE, "Will delete dir " + dir);
         if ( ! IOUtils.recursiveDeleteDir(dir))
-            log.log(Level.INFO, "Failed to delete " + dir);
+            log.log(INFO, "Failed to delete " + dir);
     }
 
     // Check if we should add file, it might already exist
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java
index dcd2720ae3e..e45c3a8e380 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java
@@ -19,6 +19,7 @@ import com.yahoo.vespa.filedistribution.FileReferenceData;
 import com.yahoo.vespa.filedistribution.FileReferenceDownload;
 import com.yahoo.vespa.filedistribution.LazyFileReferenceData;
 import com.yahoo.vespa.filedistribution.LazyTemporaryStorageFileReferenceData;
+
 import java.io.File;
 import java.io.IOException;
 import java.io.UncheckedIOException;
@@ -27,6 +28,7 @@ import java.nio.file.Path;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -42,7 +44,6 @@ import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType
 import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType.gzip;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.Type;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.Type.compressed;
-import static com.yahoo.yolean.Exceptions.uncheck;
 
 public class FileServer {
 
@@ -108,26 +109,24 @@ public class FileServer {
     }
 
     private boolean hasFile(FileReference reference) {
-        try {
-            return fileDirectory.getFile(reference).exists();
-        } catch (IllegalArgumentException e) {
-            log.log(Level.FINE, () -> "Failed locating " + reference + ": " + e.getMessage());
-        }
+        Optional<File> file = fileDirectory.getFile(reference);
+        if (file.isPresent())
+            return file.get().exists();
+
+        log.log(Level.FINE, () -> "Failed locating " + reference);
         return false;
     }
 
     FileDirectory getRootDir() { return fileDirectory; }
 
-    void startFileServing(FileReference reference, Receiver target, Set<CompressionType> acceptedCompressionTypes) {
-        File file = fileDirectory.getFile(reference);
-        if ( ! file.exists()) return;
-
+    void startFileServing(FileReference reference, File file, Receiver target, Set<CompressionType> acceptedCompressionTypes) {
+        var absolutePath = file.getAbsolutePath();
         try (FileReferenceData fileData = fileReferenceData(reference, acceptedCompressionTypes, file)) {
-            log.log(Level.FINE, () -> "Start serving " + reference.value() + " with file '" + file.getAbsolutePath() + "'");
+            log.log(Level.FINE, () -> "Start serving " + reference.value() + " with file '" + absolutePath + "'");
             target.receive(fileData, new ReplayStatus(0, "OK"));
-            log.log(Level.FINE, () -> "Done serving " + reference.value() + " with file '" + file.getAbsolutePath() + "'");
+            log.log(Level.FINE, () -> "Done serving " + reference.value() + " with file '" + absolutePath + "'");
         } catch (IOException ioe) {
-            throw new UncheckedIOException("For " + reference.value() + ": failed reading file '" + file.getAbsolutePath() + "'" +
+            throw new UncheckedIOException("For " + reference.value() + ": failed reading file '" + absolutePath + "'" +
                                            " for sending to '" + target.toString() + "'. ", ioe);
         } catch (Exception e) {
             throw new RuntimeException("Failed serving " + reference.value() + " to '" + target + "': ", e);
@@ -177,10 +176,10 @@ public class FileServer {
 
         try {
             var fileReferenceDownload = new FileReferenceDownload(fileReference, client, downloadFromOtherSourceIfNotFound);
-            boolean fileExists = hasFileDownloadIfNeeded(fileReferenceDownload);
-            if ( ! fileExists) return NOT_FOUND;
+            var file = getFileDownloadIfNeeded(fileReferenceDownload);
+            if (file.isEmpty()) return NOT_FOUND;
 
-            startFileServing(fileReference, receiver, acceptedCompressionTypes);
+            startFileServing(fileReference, file.get(), receiver, acceptedCompressionTypes);
         } catch (Exception e) {
             log.warning("Failed serving file reference '" + fileReference + "', request from " + client + " failed with: " + e.getMessage());
             return TRANSFER_FAILED;
@@ -199,9 +198,11 @@ public class FileServer {
                                            acceptedCompressionTypes + ", compression types server can use: " + compressionTypes);
     }
 
-    boolean hasFileDownloadIfNeeded(FileReferenceDownload fileReferenceDownload) {
+    public Optional<File> getFileDownloadIfNeeded(FileReferenceDownload fileReferenceDownload) {
         FileReference fileReference = fileReferenceDownload.fileReference();
-        if (hasFile(fileReference)) return true;
+        Optional<File> file = fileDirectory.getFile(fileReference);
+        if (file.isPresent())
+            return file;
 
         if (fileReferenceDownload.downloadFromOtherSourceIfNotFound()) {
             log.log(Level.FINE, "File not found, downloading from another source");
@@ -210,13 +211,13 @@ public class FileServer {
             FileReferenceDownload newDownload = new FileReferenceDownload(fileReference,
                                                                           fileReferenceDownload.client(),
                                                                           false);
-            boolean fileExists = downloader.getFile(newDownload).isPresent();
-            if ( ! fileExists)
+            file = downloader.getFile(newDownload);
+            if (file.isEmpty())
                 log.log(Level.INFO, "Failed downloading '" + fileReferenceDownload + "'");
-            return fileExists;
+            return file;
         } else {
             log.log(Level.FINE, "File not found, will not download from another source");
-            return false;
+            return Optional.empty();
         }
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
index 6ace2f5b0b8..c057bbedd5f 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
@@ -26,7 +26,6 @@ import com.yahoo.vespa.config.server.application.ApplicationSet;
 import com.yahoo.vespa.config.server.application.TenantApplications;
 import com.yahoo.vespa.config.server.configchange.ConfigChangeActions;
 import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs;
-import com.yahoo.vespa.config.server.filedistribution.FileDirectory;
 import com.yahoo.vespa.config.server.filedistribution.FileDistributionFactory;
 import com.yahoo.vespa.config.server.http.InvalidApplicationException;
 import com.yahoo.vespa.config.server.http.UnknownVespaVersionException;
@@ -857,19 +856,14 @@ public class SessionRepository {
         log.log(Level.FINE, () -> "File reference for session id " + sessionId + ": " + fileReference);
         if (fileReference.isEmpty()) return;
 
-        File sessionDir;
-        FileDirectory fileDirectory = fileDistributionFactory.fileDirectory();
-        try {
-            sessionDir = fileDirectory.getFile(fileReference.get());
-        } catch (IllegalArgumentException e) {
-            // We cannot be guaranteed that the file reference exists (it could be that it has not
-            // been downloaded yet), and e.g. when bootstrapping we cannot throw an exception in that case
-            log.log(Level.FINE, () -> "File reference for session id " + sessionId + ": " + fileReference + " not found");
-            return;
-        }
+        Optional<File> sessionDir = fileDistributionFactory.fileDirectory().getFile(fileReference.get());
+        // We cannot be guaranteed that the file reference exists (it could be that it has not
+        // been downloaded yet), and e.g. when bootstrapping we cannot throw an exception in that case
+        if (sessionDir.isEmpty()) return;
+
         ApplicationId applicationId = sessionZKClient.readApplicationId();
         log.log(Level.FINE, () -> "Creating local session for tenant '" + tenantName + "' with session id " + sessionId);
-        createLocalSession(sessionDir, applicationId, sessionId);
+        createLocalSession(sessionDir.get(), applicationId, sessionId);
     }
 
     private Optional<Long> getActiveSessionId(ApplicationId applicationId) {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDirectoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDirectoryTest.java
index 649d382ddb6..040df208323 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDirectoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDirectoryTest.java
@@ -37,9 +37,9 @@ public class FileDirectoryTest {
         FileReference foo = createFile("foo");
         FileReference bar = createFile("bar");
 
-        assertTrue(fileDirectory.getFile(foo).exists());
+        assertTrue(fileDirectory.getFile(foo).get().exists());
         assertEquals("ea315b7acac56246", foo.value());
-        assertTrue(fileDirectory.getFile(bar).exists());
+        assertTrue(fileDirectory.getFile(bar).get().exists());
         assertEquals("2b8e97f15c854e1d", bar.value());
     }
 
@@ -49,7 +49,7 @@ public class FileDirectoryTest {
         File subDirectory = new File(temporaryFolder.getRoot(), subdirName);
         createFileInSubDir(subDirectory, "foo", "some content");
         FileReference fileReference = fileDirectory.addFile(subDirectory);
-        File dir = fileDirectory.getFile(fileReference);
+        File dir = fileDirectory.getFile(fileReference).get();
         assertTrue(dir.exists());
         assertTrue(new File(dir, "foo").exists());
         assertFalse(new File(dir, "doesnotexist").exists());
@@ -58,7 +58,7 @@ public class FileDirectoryTest {
         // Change contents of a file, file reference value should change
         createFileInSubDir(subDirectory, "foo", "new content");
         FileReference fileReference2 = fileDirectory.addFile(subDirectory);
-        dir = fileDirectory.getFile(fileReference2);
+        dir = fileDirectory.getFile(fileReference2).get();
         assertTrue(new File(dir, "foo").exists());
         assertNotEquals(fileReference + " should not be equal to " + fileReference2, fileReference, fileReference2);
         assertEquals("e5d4b3fe5ee3ede3", fileReference2.value());
@@ -66,7 +66,7 @@ public class FileDirectoryTest {
         // Add a file, should be available and file reference should have another value
         createFileInSubDir(subDirectory, "bar", "some other content");
         FileReference fileReference3 = fileDirectory.addFile(subDirectory);
-        dir = fileDirectory.getFile(fileReference3);
+        dir = fileDirectory.getFile(fileReference3).get();
         assertTrue(new File(dir, "foo").exists());
         assertTrue(new File(dir, "bar").exists());
         assertEquals("894bced3fc9d199b", fileReference3.value());
@@ -78,7 +78,7 @@ public class FileDirectoryTest {
         File subDirectory = new File(temporaryFolder.getRoot(), subdirName);
         createFileInSubDir(subDirectory, "foo", "some content");
         FileReference fileReference = fileDirectory.addFile(subDirectory);
-        File dir = fileDirectory.getFile(fileReference);
+        File dir = fileDirectory.getFile(fileReference).get();
         assertTrue(dir.exists());
         File foo = new File(dir, "foo");
         assertTrue(foo.exists());
@@ -90,7 +90,7 @@ public class FileDirectoryTest {
         try { Thread.sleep(1000);} catch (InterruptedException e) {/*ignore */} // Needed since we have timestamp resolution of 1 second
         Files.delete(Paths.get(fileDirectory.getPath(fileReference)).resolve("subdir").resolve("foo"));
         fileReference = fileDirectory.addFile(subDirectory);
-        dir = fileDirectory.getFile(fileReference);
+        dir = fileDirectory.getFile(fileReference).get();
         File foo2 = new File(dir, "foo");
         assertTrue(dir.exists());
         assertTrue(foo2.exists());
@@ -107,7 +107,7 @@ public class FileDirectoryTest {
         File subDirectory = new File(temporaryFolder.getRoot(), subdirName);
         createFileInSubDir(subDirectory, "foo", "some content");
         FileReference fileReference = fileDirectory.addFile(subDirectory);
-        File dir = fileDirectory.getFile(fileReference);
+        File dir = fileDirectory.getFile(fileReference).get();
         assertTrue(dir.exists());
         File foo = new File(dir, "foo");
         assertTrue(foo.exists());
@@ -119,7 +119,7 @@ public class FileDirectoryTest {
         // Add a file that already exists, nothing should happen
         createFileInSubDir(subDirectory, "foo", "some content"); // same as before, nothing should happen
         FileReference fileReference3 = fileDirectory.addFile(subDirectory);
-        dir = fileDirectory.getFile(fileReference3);
+        dir = fileDirectory.getFile(fileReference3).get();
         assertTrue(new File(dir, "foo").exists());
         assertEquals("bebc5a1aee74223d", fileReference3.value()); // same hash
 
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileServerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileServerTest.java
index c17b68c6d12..373b39c8365 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileServerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileServerTest.java
@@ -61,9 +61,9 @@ public class FileServerTest {
         String dir = "123";
         assertFalse(fileServer.hasFile(dir));
         FileReferenceDownload foo = new FileReferenceDownload(new FileReference(dir), "test");
-        assertFalse(fileServer.hasFileDownloadIfNeeded(foo));
+        assertFalse(fileServer.getFileDownloadIfNeeded(foo).isPresent());
         writeFile(dir);
-        assertTrue(fileServer.hasFileDownloadIfNeeded(foo));
+        assertTrue(fileServer.getFileDownloadIfNeeded(foo).isPresent());
     }
 
     @Test
@@ -79,7 +79,9 @@ public class FileServerTest {
         File dir = getFileServerRootDir();
         IOUtils.writeFile(dir + "/12y/f1", "dummy-data", true);
         CompletableFuture<byte []> content = new CompletableFuture<>();
-        fileServer.startFileServing(new FileReference("12y"), new FileReceiver(content), Set.of(gzip));
+        FileReference fileReference = new FileReference("12y");
+        var file = fileServer.getFileDownloadIfNeeded(new FileReferenceDownload(fileReference, "test"));
+        fileServer.startFileServing(fileReference, file.get(), new FileReceiver(content), Set.of(gzip));
         assertEquals(new String(content.get()), "dummy-data");
     }
 
@@ -90,7 +92,9 @@ public class FileServerTest {
         File dir = getFileServerRootDir();
         IOUtils.writeFile(dir + "/subdir/12z/f1", "dummy-data-2", true);
         CompletableFuture<byte []> content = new CompletableFuture<>();
-        fileServer.startFileServing(new FileReference("subdir"), new FileReceiver(content), Set.of(gzip, lz4));
+        FileReference fileReference = new FileReference("subdir");
+        var file = fileServer.getFileDownloadIfNeeded(new FileReferenceDownload(fileReference, "test"));
+        fileServer.startFileServing(fileReference, file.get(), new FileReceiver(content), Set.of(gzip, lz4));
 
         // Decompress with lz4 and check contents
         var compressor = new FileReferenceCompressor(FileReferenceData.Type.compressed, lz4);
@@ -139,14 +143,16 @@ public class FileServerTest {
         FailingFileReceiver fileReceiver = new FailingFileReceiver(content);
 
         // Should fail the first time, see FailingFileReceiver
+        FileReference reference = new FileReference("12y");
+        var file = fileServer.getFileDownloadIfNeeded(new FileReferenceDownload(reference, "test"));
         try {
-            fileServer.startFileServing(new FileReference("12y"), fileReceiver, Set.of(gzip));
+            fileServer.startFileServing(reference, file.get(), fileReceiver, Set.of(gzip));
             fail("Should have failed");
         } catch (RuntimeException e) {
             // expected
         }
 
-        fileServer.startFileServing(new FileReference("12y"), fileReceiver, Set.of(gzip));
+        fileServer.startFileServing(reference, file.get(), fileReceiver, Set.of(gzip));
         assertEquals(new String(content.get()), "dummy-data");
     }