Merge pull request #23195 from vespa-engine/hmusum/return-response-when-file-request-times-out

Make sure to return response when request times out

4 files changed, 40 insertions, 44 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 46ae2fd15d5..d3b7e53157d 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
@@ -60,16 +60,13 @@ public class FileDirectory  {
     public File getFile(FileReference reference) {
         ensureRootExist();
         File dir = new File(getPath(reference));
-        if (!dir.exists()) {
+        if (!dir.exists())
             throw new IllegalArgumentException("File reference '" + reference.value() + "' with absolute path '" + dir.getAbsolutePath() + "' does not exist.");
-        }
-        if (!dir.isDirectory()) {
+        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) {
+        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];
     }
 
@@ -82,7 +79,7 @@ public class FileDirectory  {
         if (file.isDirectory()) {
             return Files.walk(file.toPath(), 100).map(path -> {
                 try {
-                    log.log(Level.FINE, () -> "Calculating hash for '" + path + "'");
+                    log.log(Level.FINEST, () -> "Calculating hash for '" + path + "'");
                     return hash(path.toFile(), hasher);
                 } catch (IOException e) {
                     log.log(Level.WARNING, "Failed getting hash from '" + path + "'");
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 88405b3eef9..3fe727c6f9d 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
@@ -13,10 +13,10 @@ import com.yahoo.jrt.Supervisor;
 import com.yahoo.jrt.Transport;
 import com.yahoo.vespa.config.ConnectionPool;
 import com.yahoo.vespa.defaults.Defaults;
-import com.yahoo.vespa.filedistribution.FileReferenceCompressor;
 import com.yahoo.vespa.filedistribution.EmptyFileReferenceData;
 import com.yahoo.vespa.filedistribution.FileDistributionConnectionPool;
 import com.yahoo.vespa.filedistribution.FileDownloader;
+import com.yahoo.vespa.filedistribution.FileReferenceCompressor;
 import com.yahoo.vespa.filedistribution.FileReferenceData;
 import com.yahoo.vespa.filedistribution.FileReferenceDownload;
 import com.yahoo.vespa.filedistribution.LazyFileReferenceData;
@@ -49,9 +49,11 @@ public class FileServer {
     private final ExecutorService executor;
     private final FileDownloader downloader;
 
+    // TODO: Move to filedistribution module, so that it can be used by both clients and servers
     private enum FileApiErrorCodes {
         OK(0, "OK"),
-        NOT_FOUND(1, "Filereference not found");
+        NOT_FOUND(1, "File reference not found"),
+        TIMEOUT(2, "Timeout");
         private final int code;
         private final String description;
         FileApiErrorCodes(int code, String description) {
@@ -112,13 +114,9 @@ public class FileServer {
 
     FileDirectory getRootDir() { return root; }
 
-    void startFileServing(String fileName, Receiver target) {
-        FileReference reference = new FileReference(fileName);
-        File file = root.getFile(reference);
-
-        if (file.exists()) {
-            serveFile(reference, target);
-        }
+    void startFileServing(FileReference fileReference, Receiver target) {
+        if (root.getFile(fileReference).exists())
+            serveFile(fileReference, target);
     }
 
     private void serveFile(FileReference reference, Receiver target) {
@@ -152,44 +150,43 @@ public class FileServer {
         }
     }
 
-    public void serveFile(String fileReference, boolean downloadFromOtherSourceIfNotFound, Request request, Receiver receiver) {
+    public void serveFile(FileReference fileReference, boolean downloadFromOtherSourceIfNotFound, Request request, Receiver receiver) {
         if (executor instanceof ThreadPoolExecutor)
             log.log(Level.FINE, () -> "Active threads: " + ((ThreadPoolExecutor) executor).getActiveCount());
 
         log.log(Level.FINE, () -> "Received request for file reference '" + fileReference + "' from " + request.target());
         Instant deadline = Instant.now().plus(timeout);
-        executor.execute(() -> serveFileInternal(fileReference, downloadFromOtherSourceIfNotFound, request, receiver, deadline));
-    }
-
-    private void serveFileInternal(String fileReference,
-                                   boolean downloadFromOtherSourceIfNotFound,
-                                   Request request,
-                                   Receiver receiver,
-                                   Instant deadline) {
+        String client = request.target().toString();
+        executor.execute(() -> {
+            var result = serveFileInternal(fileReference, downloadFromOtherSourceIfNotFound, client, receiver, deadline);
+            request.returnValues()
+                   .add(new Int32Value(result.getCode()))
+                   .add(new StringValue(result.getDescription()));
+            request.returnRequest();
+        });
+    }
+
+    private FileApiErrorCodes serveFileInternal(FileReference fileReference,
+                                                boolean downloadFromOtherSourceIfNotFound,
+                                                String client,
+                                                Receiver receiver,
+                                                Instant deadline) {
         if (Instant.now().isAfter(deadline)) {
-            log.log(Level.INFO, () -> "Deadline exceeded for request for file reference '" + fileReference + "' from " + request.target() +
-                    " , giving up");
-            return;
+            log.log(Level.INFO, () -> "Deadline exceeded for request for file reference '" + fileReference + "' from " + client);
+            return FileApiErrorCodes.TIMEOUT;
         }
 
         boolean fileExists;
         try {
-            String client = request.target().toString();
-            FileReferenceDownload fileReferenceDownload = new FileReferenceDownload(new FileReference(fileReference),
-                                                                                    client,
-                                                                                    downloadFromOtherSourceIfNotFound);
+            var fileReferenceDownload = new FileReferenceDownload(fileReference, client, downloadFromOtherSourceIfNotFound);
             fileExists = hasFileDownloadIfNeeded(fileReferenceDownload);
             if (fileExists) startFileServing(fileReference, receiver);
         } catch (IllegalArgumentException e) {
             fileExists = false;
-            log.warning("Failed serving file reference '" + fileReference + "', request was from " + request.target() + ", with error " + e.toString());
+            log.warning("Failed serving file reference '" + fileReference + "', request from " + client + " failed with: " + e.getMessage());
         }
 
-        FileApiErrorCodes result = fileExists ? FileApiErrorCodes.OK : FileApiErrorCodes.NOT_FOUND;
-        request.returnValues()
-                .add(new Int32Value(result.getCode()))
-                .add(new StringValue(result.getDescription()));
-        request.returnRequest();
+        return (fileExists ? FileApiErrorCodes.OK : FileApiErrorCodes.NOT_FOUND);
     }
 
     boolean hasFileDownloadIfNeeded(FileReferenceDownload fileReferenceDownload) {
@@ -199,17 +196,16 @@ public class FileServer {
         if (fileReferenceDownload.downloadFromOtherSourceIfNotFound()) {
             log.log(Level.FINE, "File not found, downloading from another source");
             // Create new FileReferenceDownload with downloadFromOtherSourceIfNotFound set to false
-            // to avoid config servers requesting a file reference perpetually, e.g. for a file that
-            // does not exist anymore
+            // to avoid requesting a file reference perpetually, e.g. for a file that does not exist anymore
             FileReferenceDownload newDownload = new FileReferenceDownload(fileReference,
                                                                           fileReferenceDownload.client(),
                                                                           false);
             boolean fileExists = downloader.getFile(newDownload).isPresent();
             if ( ! fileExists)
-                log.log(Level.WARNING, "Failed downloading '" + fileReferenceDownload + "'");
+                log.log(Level.INFO, "Failed downloading '" + fileReferenceDownload + "'");
             return fileExists;
         } else {
-            log.log(Level.FINE, "File not found, will not download from another source, since request came from another config server");
+            log.log(Level.FINE, "File not found, will not download from another source");
             return false;
         }
     }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java
index 6a8141032df..6e919ae7f2e 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java
@@ -566,7 +566,10 @@ public class RpcServer implements Runnable, ReloadListener, TenantListener {
         rpcAuthorizer.authorizeFileRequest(request)
                 .thenRun(() -> { // okay to do in authorizer thread as serveFile is async
                     FileServer.Receiver receiver = new ChunkedFileReceiver(request.target());
-                    fileServer.serveFile(request.parameters().get(0).asString(), request.parameters().get(1).asInt32() == 0, request, receiver);
+                    fileServer.serveFile(new FileReference(request.parameters().get(0).asString()),
+                                         request.parameters().get(1).asInt32() == 0,
+                                         request,
+                                         receiver);
                 });
     }
 
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 67c40f94b6a..861e6967620 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
@@ -75,7 +75,7 @@ public class FileServerTest {
         File dir = getFileServerRootDir();
         IOUtils.writeFile(dir + "/12y/f1", "dummy-data", true);
         CompletableFuture<byte []> content = new CompletableFuture<>();
-        fileServer.startFileServing("12y", new FileReceiver(content));
+        fileServer.startFileServing(new FileReference("12y"), new FileReceiver(content));
         assertEquals(new String(content.get()), "dummy-data");
     }