Merge pull request #32220 from vespa-engine/hmusum/move-filedistribut…

…ion-error-codes Hmusum/move filedistribution error codes
vespa-engine · Aug 22, 2024 · 482ebfb · 482ebfb
2 parents f13cb6b + 931e022
commit 482ebfb
Show file tree

Hide file tree

Showing 10 changed files with 54 additions and 72 deletions.
diff --git a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java
@@ -60,8 +60,6 @@ public void applyPayload(ConfigPayload payload) {
         stack.push(new NamedBuilder(rootBuilder));
         try {
             handleValue(payload.getSlime().get());
-        } catch (FileReferenceDoesNotExistException e) {
-            throw e;
         } catch (Exception e) {
             throw new RuntimeException("Not able to create config builder for payload '" + payload.toString() + "'", e);
         }

diff --git a/config/src/main/java/com/yahoo/vespa/config/FileReferenceDoesNotExistException.java b/config/src/main/java/com/yahoo/vespa/config/FileReferenceDoesNotExistException.java
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
@@ -12,6 +12,7 @@
 import com.yahoo.jrt.Supervisor;
 import com.yahoo.jrt.Transport;
 import com.yahoo.vespa.config.ConnectionPool;
+import com.yahoo.vespa.filedistribution.FileApiErrorCodes;
 import com.yahoo.vespa.filedistribution.FileDistributionConnectionPool;
 import com.yahoo.vespa.filedistribution.FileDownloader;
 import com.yahoo.vespa.filedistribution.FileReferenceCompressor;
@@ -36,11 +37,13 @@
 import java.util.logging.Logger;
 
 import static com.yahoo.vespa.config.server.filedistribution.FileDistributionUtil.getOtherConfigServersInCluster;
-import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.NOT_FOUND;
-import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.OK;
-import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.TRANSFER_FAILED;
+import static com.yahoo.vespa.filedistribution.FileApiErrorCodes.NOT_FOUND;
+import static com.yahoo.vespa.filedistribution.FileApiErrorCodes.OK;
+import static com.yahoo.vespa.filedistribution.FileApiErrorCodes.TRANSFER_FAILED;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType.gzip;
+import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType.lz4;
+import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType.zstd;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.Type;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.Type.compressed;
 import static com.yahoo.yolean.Exceptions.uncheck;
@@ -51,7 +54,7 @@ public class FileServer {
 
     // Set this low, to make sure we don't wait for a long time trying to download file
     private static final Duration timeout = Duration.ofSeconds(10);
-    private static final List<CompressionType> compressionTypesToServe = compressionTypesAsList(List.of("zstd", "lz4", "gzip")); // In preferred order
+    private static final List<CompressionType> compressionTypesToServe = List.of(zstd, lz4, gzip); // In preferred order
     private static final String tempFilereferencedataPrefix = "filereferencedata";
     private static final Path tempFilereferencedataDir = Paths.get(System.getProperty("java.io.tmpdir"));
 
@@ -60,22 +63,6 @@ public class FileServer {
     private final FileDownloader downloader;
     private final List<CompressionType> compressionTypes; // compression types to use, in preferred order
 
-    // TODO: Move to filedistribution module, so that it can be used by both clients and servers
-    enum FileApiErrorCodes {
-        OK(0, "OK"),
-        NOT_FOUND(1, "File reference not found"),
-        TIMEOUT(2, "Timeout"),
-        TRANSFER_FAILED(3, "Failed transferring file");
-        private final int code;
-        private final String description;
-        FileApiErrorCodes(int code, String description) {
-            this.code = code;
-            this.description = description;
-        }
-        int getCode() { return code; }
-        String getDescription() { return description; }
-    }
-
     public static class ReplayStatus {
         private final int code;
         private final String description;
@@ -165,8 +152,8 @@ public void serveFile(FileReference fileReference,
         executor.execute(() -> {
             var result = serveFileInternal(fileReference, downloadFromOtherSourceIfNotFound, client, receiver, acceptedCompressionTypes);
             request.returnValues()
-                   .add(new Int32Value(result.getCode()))
-                   .add(new StringValue(result.getDescription()));
+                   .add(new Int32Value(result.code()))
+                   .add(new StringValue(result.description()));
             request.returnRequest();
         });
     }
@@ -235,12 +222,6 @@ private static FileDownloader createFileDownloader(List<String> configServers) {
         return new FileDownloader(createConnectionPool(configServers, supervisor), supervisor, timeout);
     }
 
-    private static List<CompressionType> compressionTypesAsList(List<String> compressionTypes) {
-        return compressionTypes.stream()
-                               .map(CompressionType::valueOf)
-                               .toList();
-    }
-
     private static ConnectionPool createConnectionPool(List<String> configServers, Supervisor supervisor) {
         if (configServers.isEmpty()) return FileDownloader.emptyConnectionPool();
 

diff --git a/fileacquirer/abi-spec.json b/fileacquirer/abi-spec.json
@@ -39,7 +39,6 @@
       "public static com.yahoo.filedistribution.fileacquirer.FileAcquirer returnFile(java.io.File)",
       "public static com.yahoo.filedistribution.fileacquirer.FileAcquirer returnFiles(java.util.Map)",
       "public static com.yahoo.filedistribution.fileacquirer.FileAcquirer throwTimeoutException()",
-      "public static com.yahoo.filedistribution.fileacquirer.FileAcquirer throwFileReferenceDoesNotExistException()",
       "public void shutdown()"
     ],
     "fields" : [ ]

diff --git a/fileacquirer/src/main/java/com/yahoo/filedistribution/fileacquirer/FileAcquirer.java b/fileacquirer/src/main/java/com/yahoo/filedistribution/fileacquirer/FileAcquirer.java
@@ -2,7 +2,6 @@
 package com.yahoo.filedistribution.fileacquirer;
 
 import com.yahoo.config.FileReference;
-import com.yahoo.vespa.config.FileReferenceDoesNotExistException;
 
 import java.io.File;
 import java.util.concurrent.TimeUnit;
@@ -22,8 +21,6 @@ public interface FileAcquirer {
      * config system.
      *
      * @throws TimeoutException if the file or directory could not be retrieved in time.
-     * @throws FileReferenceDoesNotExistException if the file is no
-     *         longer available(due to reloading of config).
      */
     File waitFor(FileReference fileReference, long timeout, TimeUnit timeUnit) throws InterruptedException;
 

diff --git a/fileacquirer/src/main/java/com/yahoo/filedistribution/fileacquirer/FileAcquirerImpl.java b/fileacquirer/src/main/java/com/yahoo/filedistribution/fileacquirer/FileAcquirerImpl.java
@@ -8,7 +8,6 @@
 import com.yahoo.jrt.Supervisor;
 import com.yahoo.jrt.Target;
 import com.yahoo.jrt.Transport;
-import com.yahoo.vespa.config.FileReferenceDoesNotExistException;
 
 import java.io.File;
 import java.time.Duration;
@@ -133,7 +132,6 @@ public void shutdown() {
      * config system.
      *
      * @throws TimeoutException if the file or directory could not be retrieved in time.
-     * @throws FileReferenceDoesNotExistException if the file is no longer available (due to reloading of config).
      */
     public File waitFor(FileReference fileReference, long timeout, TimeUnit timeUnit) throws InterruptedException {
         Timer timer = new Timer(timeout, timeUnit);

diff --git a/fileacquirer/src/main/java/com/yahoo/filedistribution/fileacquirer/MockFileAcquirer.java b/fileacquirer/src/main/java/com/yahoo/filedistribution/fileacquirer/MockFileAcquirer.java
@@ -2,7 +2,6 @@
 package com.yahoo.filedistribution.fileacquirer;
 
 import com.yahoo.config.FileReference;
-import com.yahoo.vespa.config.FileReferenceDoesNotExistException;
 import java.io.File;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
@@ -43,16 +42,6 @@ public File waitFor(FileReference fileReference, long timeout, TimeUnit timeUnit
         };
     }
 
-    /** Creates a FileAcquirer that throws FileReferenceDoesNotExistException **/
-    public static FileAcquirer throwFileReferenceDoesNotExistException() {
-        return new MockFileAcquirer() {
-            @Override
-            public File waitFor(FileReference fileReference, long timeout, TimeUnit timeUnit) {
-                throw new FileReferenceDoesNotExistException(null);
-            }
-        };
-    }
-
     @Override
     public void shutdown() {}
 

diff --git a/fileacquirer/src/test/java/MockFileAcquirerTest.java b/fileacquirer/src/test/java/MockFileAcquirerTest.java
@@ -4,7 +4,6 @@
 import com.yahoo.filedistribution.fileacquirer.FileAcquirer;
 import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer;
 import com.yahoo.filedistribution.fileacquirer.TimeoutException;
-import com.yahoo.vespa.config.FileReferenceDoesNotExistException;
 import org.junit.Test;
 import java.io.File;
 import java.lang.reflect.Constructor;
@@ -46,11 +45,6 @@ public void testThrowTimeoutException() throws Exception {
         waitFor(MockFileAcquirer.throwTimeoutException());
     }
 
-    @Test(expected = FileReferenceDoesNotExistException.class)
-    public void testThrowFileReferenceDoesNotExistException() throws Exception {
-        waitFor(MockFileAcquirer.throwFileReferenceDoesNotExistException());
-    }
-
     private File waitFor(FileAcquirer fileAcquirer) throws InterruptedException {
         return waitFor(fileAcquirer, null);
     }

diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileApiErrorCodes.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileApiErrorCodes.java
@@ -0,0 +1,39 @@
+// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.filedistribution;
+
+/**
+ * Error codes used in RPC responses for file distribution
+ *
+ * @author hmusum
+ */
+public enum FileApiErrorCodes {
+
+    OK(0, "OK"),
+    NOT_FOUND(1, "File reference not found"),
+    TIMEOUT(2, "Timeout"),
+    TRANSFER_FAILED(3, "Failed transferring file");
+    private final int code;
+    private final String description;
+
+    FileApiErrorCodes(int code, String description) {
+        this.code = code;
+        this.description = description;
+    }
+
+    static FileApiErrorCodes get(int code) {
+        for (FileApiErrorCodes error : FileApiErrorCodes.values()) {
+            if (error.code() == code) {
+                return error;
+            }
+        }
+        return null;
+    }
+
+    public int code() { return code; }
+
+    public String description() { return description; }
+
+    @Override
+    public String toString() { return code + ": " + description; }
+
+}
diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
@@ -151,18 +151,21 @@ private boolean startDownloadRpc(FileReferenceDownload fileReferenceDownload, in
 
         Level logLevel = (retryCount > 3 ? Level.INFO : Level.FINE);
         FileReference fileReference = fileReferenceDownload.fileReference();
+        String address = connection.getAddress();
         if (validateResponse(request)) {
             log.log(Level.FINE, () -> "Request callback, OK. Req: " + request + "\nSpec: " + connection);
             int errorCode = request.returnValues().get(0).asInt32();
+
             if (errorCode == 0) {
-                log.log(Level.FINE, () -> "Found " + fileReference + " available at " + connection.getAddress());
+                log.log(Level.FINE, () -> "Found " + fileReference + " available at " + address);
                 return true;
             } else {
-                log.log(logLevel, fileReference + " not found or timed out (error code " +  errorCode + ") at " + connection.getAddress());
+                var error = FileApiErrorCodes.get(errorCode);
+                log.log(logLevel, "Downloading " + fileReference + " from " + address + " failed (" + error + ")");
                 return false;
             }
         } else {
-            log.log(logLevel, "Downloading " + fileReference + " from " + connection.getAddress() + " failed:" +
+            log.log(logLevel, "Downloading " + fileReference + " from " + address + " failed:" +
                     " error code " + request.errorCode() + " (" + request.errorMessage() + ")." +
                     " (retry " + retryCount + ", rpc timeout " + rpcTimeout + ")");
             return false;