From 1a38f5333d0285efd12b3f575b7de8100c6419c6 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 16 Aug 2023 15:25:41 +0200 Subject: Return optional from getFile --- .../server/filedistribution/FileDirectory.java | 31 +++++++++++++--------- .../config/server/filedistribution/FileServer.java | 25 ++++++++--------- .../config/server/session/SessionRepository.java | 18 +++++-------- .../server/filedistribution/FileDirectoryTest.java | 18 ++++++------- 4 files changed, 47 insertions(+), 45 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 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 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..a26d87743f0 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 @@ -27,6 +27,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 +43,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 +108,27 @@ 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 = 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 acceptedCompressionTypes) { - File file = fileDirectory.getFile(reference); - if ( ! file.exists()) return; + Optional file = fileDirectory. getFile(reference); + if (file.isEmpty()) return; - try (FileReferenceData fileData = fileReferenceData(reference, acceptedCompressionTypes, file)) { - log.log(Level.FINE, () -> "Start serving " + reference.value() + " with file '" + file.getAbsolutePath() + "'"); + var absolutePath = file.get().getAbsolutePath(); + try (FileReferenceData fileData = fileReferenceData(reference, acceptedCompressionTypes, file.get())) { + 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); 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 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 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 -- cgit v1.2.3 From 618bb8d5fbe67fedbcd993a3f7744f9a72ca3635 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 16 Aug 2023 15:46:26 +0200 Subject: Return optional of File instead of boolean --- .../config/server/filedistribution/FileServer.java | 30 +++++++++++----------- .../server/filedistribution/FileServerTest.java | 18 ++++++++----- 2 files changed, 27 insertions(+), 21 deletions(-) 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 a26d87743f0..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; @@ -118,12 +119,9 @@ public class FileServer { FileDirectory getRootDir() { return fileDirectory; } - void startFileServing(FileReference reference, Receiver target, Set acceptedCompressionTypes) { - Optional file = fileDirectory. getFile(reference); - if (file.isEmpty()) return; - - var absolutePath = file.get().getAbsolutePath(); - try (FileReferenceData fileData = fileReferenceData(reference, acceptedCompressionTypes, file.get())) { + void startFileServing(FileReference reference, File file, Receiver target, Set acceptedCompressionTypes) { + var absolutePath = file.getAbsolutePath(); + try (FileReferenceData fileData = fileReferenceData(reference, acceptedCompressionTypes, file)) { 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 '" + absolutePath + "'"); @@ -178,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; @@ -200,9 +198,11 @@ public class FileServer { acceptedCompressionTypes + ", compression types server can use: " + compressionTypes); } - boolean hasFileDownloadIfNeeded(FileReferenceDownload fileReferenceDownload) { + public Optional getFileDownloadIfNeeded(FileReferenceDownload fileReferenceDownload) { FileReference fileReference = fileReferenceDownload.fileReference(); - if (hasFile(fileReference)) return true; + Optional file = fileDirectory.getFile(fileReference); + if (file.isPresent()) + return file; if (fileReferenceDownload.downloadFromOtherSourceIfNotFound()) { log.log(Level.FINE, "File not found, downloading from another source"); @@ -211,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/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 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 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"); } -- cgit v1.2.3