Minor refactoring

3 files changed, 17 insertions, 22 deletions

diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java
index 24b3fcac3e3..9b9867326d7 100644
--- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java
+++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java
@@ -60,16 +60,13 @@ public class FileDownloader {
     private Future<Optional<File>> getFutureFile(FileReferenceDownload fileReferenceDownload) {
         FileReference fileReference = fileReferenceDownload.fileReference();
         Objects.requireNonNull(fileReference, "file reference cannot be null");
-        log.log(LogLevel.DEBUG, () -> "Checking if file reference '" + fileReference.value() + "' exists in '" +
-                downloadDirectory.getAbsolutePath() + "' ");
+
         Optional<File> file = getFileFromFileSystem(fileReference, downloadDirectory);
         if (file.isPresent()) {
             SettableFuture<Optional<File>> future = SettableFuture.create();
             future.set(file);
             return future;
         } else {
-            log.log(LogLevel.DEBUG, () -> "File reference '" + fileReference.value() + "' not found in " +
-                    downloadDirectory.getAbsolutePath() + ", starting download");
             return download(fileReferenceDownload);
         }
     }
@@ -86,6 +83,7 @@ public class FileDownloader {
         return downloadDirectory;
     }
 
+    // Files are moved atomically, so if file reference exists and is accessible we can use it
     private Optional<File> getFileFromFileSystem(FileReference fileReference, File directory) {
         File[] files = new File(directory, fileReference.value()).listFiles();
         if (directory.exists() && directory.isDirectory() && files != null && files.length > 0) {
@@ -111,27 +109,18 @@ public class FileDownloader {
         }
     }
 
-    public boolean downloadIfNeeded(FileReferenceDownload fileReferenceDownload) {
-        if (!alreadyDownloaded(fileReferenceDownload.fileReference())) {
-            download(fileReferenceDownload);
-            return true;
-        } else {
-            log.log(LogLevel.DEBUG, () -> "Download not needed, " + fileReferenceDownload.fileReference() + " already downloaded" );
-            return false;
-        }
+    /** Start a download, don't wait for result */
+    public void downloadIfNeeded(FileReferenceDownload fileReferenceDownload) {
+        FileReference fileReference = fileReferenceDownload.fileReference();
+        if (alreadyDownloaded(fileReference) || fileReferenceDownloader.isDownloading(fileReference)) return;
+
+        queueForDownload(fileReferenceDownload);
     }
 
     private synchronized Future<Optional<File>> download(FileReferenceDownload fileReferenceDownload) {
         FileReference fileReference = fileReferenceDownload.fileReference();
         Future<Optional<File>> inProgress = fileReferenceDownloader.addDownloadListener(fileReference, () -> getFile(fileReferenceDownload));
-        if (inProgress != null) {
-            log.log(LogLevel.DEBUG, () -> "Already downloading '" + fileReference.value() + "'");
-            return inProgress;
-        }
-
-        Future<Optional<File>> future = queueForDownload(fileReferenceDownload);
-        log.log(LogLevel.DEBUG, () -> "Queued '" + fileReference.value() + "' for download with timeout " + timeout);
-        return future;
+        return (inProgress == null) ? queueForDownload(fileReferenceDownload) : inProgress;
     }
 
     private Future<Optional<File>> queueForDownload(FileReferenceDownload fileReferenceDownload) {
diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
index 8a0212211e1..ae0040cc7dd 100644
--- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
+++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
@@ -40,7 +40,9 @@ public class FileReferenceDownloader {
             Executors.newFixedThreadPool(Math.max(8, Runtime.getRuntime().availableProcessors()),
                                          new DaemonThreadFactory("filereference downloader"));
     private final ConnectionPool connectionPool;
+    /* Ongoing downloads */
     private final Map<FileReference, FileReferenceDownload> downloads = new LinkedHashMap<>();
+    /* Status for ongoing and finished downloads */
     private final Map<FileReference, Double> downloadStatus = new HashMap<>();  // between 0 and 1
     private final Duration downloadTimeout;
     private final Duration sleepBetweenRetries;
diff --git a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java
index d1d12cb07b7..92f5cd51d87 100644
--- a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java
+++ b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java
@@ -197,11 +197,15 @@ public class FileDownloaderTest {
         FileDownloader fileDownloader = new FileDownloader(connectionPool, downloadDir, tempDir, timeout, sleepBetweenRetries);
         FileReference foo = new FileReference("foo");
         // Should download since we do not have the file on disk
-        assertTrue(fileDownloader.downloadIfNeeded(new FileReferenceDownload(foo)));
+        fileDownloader.downloadIfNeeded(new FileReferenceDownload(foo));
+        assertTrue(fileDownloader.fileReferenceDownloader().isDownloading(foo));
+        assertFalse(fileDownloader.getFile(foo).isPresent());
         // Receive files to simulate download
         receiveFile();
         // Should not download, since file has already been downloaded
-        assertFalse(fileDownloader.downloadIfNeeded(new FileReferenceDownload(foo)));
+        fileDownloader.downloadIfNeeded(new FileReferenceDownload(foo));
+        // and file should be available
+        assertTrue(fileDownloader.getFile(foo).isPresent());
     }
 
     @Test