diff options
7 files changed, 171 insertions, 61 deletions
diff --git a/config-model/src/main/java/com/yahoo/schema/Schema.java b/config-model/src/main/java/com/yahoo/schema/Schema.java index 93bec4975a6..36730a502ea 100644 --- a/config-model/src/main/java/com/yahoo/schema/Schema.java +++ b/config-model/src/main/java/com/yahoo/schema/Schema.java @@ -319,16 +319,12 @@ public class Schema implements ImmutableSchema { return null; } - /** - * @return true if the document has been added. - */ + /** Returns true if the document has been added. */ public boolean hasDocument() { return documentType != null; } - /** - * @return The document in this search. - */ + /** Returns the document in this search. */ @Override public SDDocumentType getDocument() { return documentType; @@ -384,7 +380,7 @@ public class Schema implements ImmutableSchema { } /** - * Returns a field defined in one of the documents of this search definition. + * Returns a field defined in one of the documents of this schema. * This does not include the extra fields defined outside the document * (those accessible through the getExtraField() method). * diff --git a/config-model/src/main/java/com/yahoo/schema/document/ImmutableSDField.java b/config-model/src/main/java/com/yahoo/schema/document/ImmutableSDField.java index 2d826e164b7..4c7e7eb28f4 100644 --- a/config-model/src/main/java/com/yahoo/schema/document/ImmutableSDField.java +++ b/config-model/src/main/java/com/yahoo/schema/document/ImmutableSDField.java @@ -101,4 +101,5 @@ public interface ImmutableSDField { boolean existsIndex(String name); SummaryField getSummaryField(String name); boolean hasIndex(); + } diff --git a/config-model/src/main/java/com/yahoo/schema/processing/IndexingInputs.java b/config-model/src/main/java/com/yahoo/schema/processing/IndexingInputs.java index 88e84d5289f..985ec8653c7 100644 --- a/config-model/src/main/java/com/yahoo/schema/processing/IndexingInputs.java +++ b/config-model/src/main/java/com/yahoo/schema/processing/IndexingInputs.java @@ -96,11 +96,11 @@ public class IndexingInputs extends Processor { @Override protected void doVisit(Expression exp) { if ( ! (exp instanceof InputExpression)) return; - String inputField = ((InputExpression)exp).getFieldName(); - if (schema.getField(inputField).hasFullIndexingDocprocRights()) return; - - fail(schema, field, "Indexing script refers to field '" + inputField + "' which does not exist " + - "in document type '" + schema.getDocument().getName() + "', and is not a mutable attribute."); + var referencedFieldName = ((InputExpression)exp).getFieldName(); + var referencedField = schema.getField(referencedFieldName); + if (referencedField == null || ! referencedField.hasFullIndexingDocprocRights()) + fail(schema, field, "Indexing script refers to field '" + referencedFieldName + + "' which is neither a field in " + schema.getDocument() + " nor a mutable attribute"); } } } diff --git a/config-model/src/test/java/com/yahoo/schema/processing/IndexingInputsTestCase.java b/config-model/src/test/java/com/yahoo/schema/processing/IndexingInputsTestCase.java index 893ee3b1ea4..d420623f233 100644 --- a/config-model/src/test/java/com/yahoo/schema/processing/IndexingInputsTestCase.java +++ b/config-model/src/test/java/com/yahoo/schema/processing/IndexingInputsTestCase.java @@ -17,29 +17,29 @@ public class IndexingInputsTestCase { void requireThatExtraFieldInputExtraFieldThrows() throws IOException, ParseException { assertBuildFails("src/test/examples/indexing_extra_field_input_extra_field.sd", "For schema 'indexing_extra_field_input_extra_field', field 'bar': Indexing script refers " + - "to field 'bar' which does not exist in document type " + - "'indexing_extra_field_input_extra_field', and is not a mutable attribute."); + "to field 'bar' which is neither a field in document type " + + "'indexing_extra_field_input_extra_field' nor a mutable attribute"); } @Test void requireThatExtraFieldInputImplicitThrows() throws IOException, ParseException { assertBuildFails("src/test/examples/indexing_extra_field_input_implicit.sd", "For schema 'indexing_extra_field_input_implicit', field 'foo': Indexing script refers to " + - "field 'foo' which does not exist in document type 'indexing_extra_field_input_implicit', and is not a mutable attribute."); + "field 'foo' which is neither a field in document type 'indexing_extra_field_input_implicit' nor a mutable attribute"); } @Test void requireThatExtraFieldInputNullThrows() throws IOException, ParseException { assertBuildFails("src/test/examples/indexing_extra_field_input_null.sd", "For schema 'indexing_extra_field_input_null', field 'foo': Indexing script refers to field " + - "'foo' which does not exist in document type 'indexing_extra_field_input_null', and is not a mutable attribute."); + "'foo' which is neither a field in document type 'indexing_extra_field_input_null' nor a mutable attribute"); } @Test void requireThatExtraFieldInputSelfThrows() throws IOException, ParseException { assertBuildFails("src/test/examples/indexing_extra_field_input_self.sd", "For schema 'indexing_extra_field_input_self', field 'foo': Indexing script refers to field " + - "'foo' which does not exist in document type 'indexing_extra_field_input_self', and is not a mutable attribute."); + "'foo' which is neither a field in document type 'indexing_extra_field_input_self' nor a mutable attribute"); } } diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainer.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainer.java index 627a15aab65..eab1368a2a1 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainer.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainer.java @@ -3,24 +3,29 @@ package com.yahoo.vespa.config.proxy.filedistribution; import com.yahoo.concurrent.DaemonThreadFactory; import com.yahoo.io.IOUtils; +import com.yahoo.vespa.config.util.ConfigUtils; import com.yahoo.vespa.filedistribution.FileDownloader; + import java.io.File; import java.io.IOException; import java.io.UncheckedIOException; +import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributes; import java.time.Duration; import java.time.Instant; -import java.util.Arrays; +import java.util.Comparator; import java.util.HashSet; +import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; -import java.util.stream.Collectors; import static java.nio.file.Files.readAttributes; +import static java.util.logging.Level.INFO; /** * Deletes file references and url downloads that have not been used for some time. @@ -35,27 +40,43 @@ class FileReferencesAndDownloadsMaintainer implements Runnable { private static final File defaultUrlDownloadDir = UrlDownloadRpcServer.downloadDir; private static final File defaultFileReferencesDownloadDir = FileDownloader.defaultDownloadDirectory; private static final Duration defaultDurationToKeepFiles = Duration.ofDays(30); + private static final int defaultOutdatedFilesToKeep = 20; private static final Duration interval = Duration.ofMinutes(1); - private final ScheduledExecutorService executor = - new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("file references and downloads cleanup")); + private final Optional<ScheduledExecutorService> executor; private final File urlDownloadDir; private final File fileReferencesDownloadDir; private final Duration durationToKeepFiles; + private final int outDatedFilesToKeep; FileReferencesAndDownloadsMaintainer() { - this(defaultFileReferencesDownloadDir, defaultUrlDownloadDir, keepFileReferencesDuration()); + this(defaultFileReferencesDownloadDir, defaultUrlDownloadDir, keepFileReferencesDuration(), + outDatedFilesToKeep(), configServers()); } - FileReferencesAndDownloadsMaintainer(File fileReferencesDownloadDir, File urlDownloadDir, Duration durationToKeepFiles) { + FileReferencesAndDownloadsMaintainer(File fileReferencesDownloadDir, + File urlDownloadDir, + Duration durationToKeepFiles, + int outdatedFilesToKeep, + List<String> configServers) { this.fileReferencesDownloadDir = fileReferencesDownloadDir; this.urlDownloadDir = urlDownloadDir; this.durationToKeepFiles = durationToKeepFiles; - executor.scheduleAtFixedRate(this, interval.toSeconds(), interval.toSeconds(), TimeUnit.SECONDS); + this.outDatedFilesToKeep = outdatedFilesToKeep; + // Do not run on config servers + if (configServers.contains(ConfigUtils.getCanonicalHostName())) { + log.log(INFO, "Not running maintainer, since this is on a config server host"); + executor = Optional.empty(); + } else { + executor = Optional.of(new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("file references and downloads cleanup"))); + executor.get().scheduleAtFixedRate(this, interval.toSeconds(), interval.toSeconds(), TimeUnit.SECONDS); + } } @Override public void run() { + if (executor.isEmpty()) return; + try { deleteUnusedFiles(fileReferencesDownloadDir); deleteUnusedFiles(urlDownloadDir); @@ -65,42 +86,62 @@ class FileReferencesAndDownloadsMaintainer implements Runnable { } public void close() { - executor.shutdownNow(); - try { - if ( ! executor.awaitTermination(10, TimeUnit.SECONDS)) - throw new RuntimeException("Unable to shutdown " + executor + " before timeout"); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } + executor.ifPresent(ex -> { + ex.shutdownNow(); + try { + if (! ex.awaitTermination(10, TimeUnit.SECONDS)) + throw new RuntimeException("Unable to shutdown " + executor + " before timeout"); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); } private void deleteUnusedFiles(File directory) { - Instant deleteNotUsedSinceInstant = Instant.now().minus(durationToKeepFiles); - Set<String> filesOnDisk = new HashSet<>(); + File[] files = directory.listFiles(); - if (files != null) - filesOnDisk.addAll(Arrays.stream(files).map(File::getName).collect(Collectors.toSet())); - log.log(Level.FINE, () -> "Files on disk (in " + directory + "): " + filesOnDisk); + if (files == null) return; + + List<File> filesToDelete = filesThatCanBeDeleted(files); + filesToDelete.forEach(fileReference -> { + if (IOUtils.recursiveDeleteDir(fileReference)) + log.log(Level.FINE, "Deleted " + fileReference.getAbsolutePath()); + else + log.log(Level.WARNING, "Could not delete " + fileReference.getAbsolutePath()); + }); + } - Set<String> filesToDelete = filesOnDisk + private List<File> filesThatCanBeDeleted(File[] files) { + Instant deleteNotUsedSinceInstant = Instant.now().minus(durationToKeepFiles); + + Set<File> filesOnDisk = new HashSet<>(List.of(files)); + log.log(Level.FINE, () -> "Files on disk: " + filesOnDisk); + int deleteCount = Math.max(0, filesOnDisk.size() - outDatedFilesToKeep); + var canBeDeleted = filesOnDisk .stream() - .filter(fileReference -> isFileLastModifiedBefore(new File(directory, fileReference), deleteNotUsedSinceInstant)) - .collect(Collectors.toSet()); - if (filesToDelete.size() > 0) { - log.log(Level.INFO, "Files that can be deleted in " + directory + " (not used since " + deleteNotUsedSinceInstant + "): " + filesToDelete); - filesToDelete.forEach(fileReference -> { - File file = new File(directory, fileReference); - if (!IOUtils.recursiveDeleteDir(file)) - log.log(Level.WARNING, "Could not delete " + file.getAbsolutePath()); - }); - } + .peek(file -> log.log(Level.FINE, () -> file + ":" + fileLastModifiedTime(file.toPath()))) + .filter(fileReference -> isFileLastModifiedBefore(fileReference, deleteNotUsedSinceInstant)) + .sorted(Comparator.comparing(fileReference -> fileLastModifiedTime(fileReference.toPath()))) + .toList(); + + // Make sure we keep some files + canBeDeleted = canBeDeleted.subList(0, Math.min(canBeDeleted.size(), deleteCount)); + if (canBeDeleted.size() > 0) + log.log(INFO, "Files that can be deleted (not accessed since " + deleteNotUsedSinceInstant + + ", will also keep " + outDatedFilesToKeep + + " no matter when last accessed): " + canBeDeleted); + + return canBeDeleted; } private boolean isFileLastModifiedBefore(File fileReference, Instant instant) { - BasicFileAttributes fileAttributes; + return fileLastModifiedTime(fileReference.toPath()).isBefore(instant); + } + + private static Instant fileLastModifiedTime(Path fileReference) { try { - fileAttributes = readAttributes(fileReference.toPath(), BasicFileAttributes.class); - return fileAttributes.lastModifiedTime().toInstant().isBefore(instant); + BasicFileAttributes fileAttributes = readAttributes(fileReference, BasicFileAttributes.class); + return fileAttributes.lastModifiedTime().toInstant(); } catch (IOException e) { throw new UncheckedIOException(e); } @@ -114,4 +155,21 @@ class FileReferencesAndDownloadsMaintainer implements Runnable { return defaultDurationToKeepFiles; } + private static int outDatedFilesToKeep() { + String env = System.getenv("VESPA_KEEP_FILE_REFERENCES_COUNT"); + if (env != null && !env.isEmpty()) + return Integer.parseInt(env); + else + return defaultOutdatedFilesToKeep; + } + + private static List<String> configServers() { + String env = System.getenv("VESPA_CONFIGSERVERS"); + if (env == null || env.isEmpty()) + return List.of(ConfigUtils.getCanonicalHostName()); + else { + return List.of(env.split(",")); + } + } + } diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainerTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainerTest.java index fad021c0119..c41305b4dc8 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainerTest.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/filedistribution/FileReferencesAndDownloadsMaintainerTest.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.config.proxy.filedistribution; import com.yahoo.io.IOUtils; +import com.yahoo.vespa.config.util.ConfigUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -10,6 +11,9 @@ import java.io.File; import java.io.IOException; import java.time.Duration; import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.IntStream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -19,9 +23,12 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; */ public class FileReferencesAndDownloadsMaintainerTest { + private static final Duration keepDuration = Duration.ofMinutes(1); + private static final int outDatedFilesToKeep = 9; + private File cachedFileReferences; private File cachedDownloads; - private FileReferencesAndDownloadsMaintainer cachedFilesMaintainer; + private FileReferencesAndDownloadsMaintainer maintainer; @TempDir public File tempFolder; @@ -30,22 +37,70 @@ public class FileReferencesAndDownloadsMaintainerTest { public void setup() throws IOException { cachedFileReferences = newFolder(tempFolder, "cachedFileReferences"); cachedDownloads = newFolder(tempFolder, "cachedDownloads"); - cachedFilesMaintainer = new FileReferencesAndDownloadsMaintainer(cachedFileReferences, cachedDownloads, Duration.ofMinutes(1)); } @Test - void require_old_files_to_be_deleted() throws IOException { + void require_old_files_to_be_deleted() { + maintainer = new FileReferencesAndDownloadsMaintainer(cachedFileReferences, cachedDownloads, keepDuration, outDatedFilesToKeep, + List.of("host1")); runMaintainerAndAssertFiles(0, 0); - File fileReference = writeFile(cachedFileReferences, "fileReference"); - File download = writeFile(cachedDownloads, "download"); - runMaintainerAndAssertFiles(1, 1); + var fileReferences = writeFiles(20); + var downloads = writeDownloads(21); + runMaintainerAndAssertFiles(20, 21); + + updateLastModifiedTimestamp(0, 5, fileReferences, downloads); + runMaintainerAndAssertFiles(15, 16); - updateLastModifiedTimeStamp(fileReference, Instant.now().minus(Duration.ofMinutes(10))); - runMaintainerAndAssertFiles(0, 1); + updateLastModifiedTimestamp(6, 20, fileReferences, downloads); + // Should keep at least outDatedFilesToKeep file references and downloads even if there are more that are old + runMaintainerAndAssertFiles(outDatedFilesToKeep, outDatedFilesToKeep); + } - updateLastModifiedTimeStamp(download, Instant.now().minus(Duration.ofMinutes(10))); + @Test + void require_no_files_deleted_when_running_on_config_server_host() { + maintainer = new FileReferencesAndDownloadsMaintainer(cachedFileReferences, cachedDownloads, keepDuration, + outDatedFilesToKeep, List.of(ConfigUtils.getCanonicalHostName())); runMaintainerAndAssertFiles(0, 0); + + var fileReferences = writeFiles(10); + var downloads = writeDownloads(10); + runMaintainerAndAssertFiles(10, 10); + + updateLastModifiedTimestamp(0, 10, fileReferences, downloads); + runMaintainerAndAssertFiles(10, 10); + } + + private void updateLastModifiedTimestamp(int startInclusive, int endExclusive, List<File> fileReferences, List<File> downloads) { + IntStream.range(startInclusive, endExclusive).forEach(i -> { + Instant instant = Instant.now().minus(keepDuration.plus(Duration.ofMinutes(1)).minus(Duration.ofSeconds(i))); + updateLastModifiedTimeStamp(fileReferences.get(i), instant); + updateLastModifiedTimeStamp(downloads.get(i), instant); + }); + } + + private List<File> writeFiles(int count) { + List<File> files = new ArrayList<>(); + IntStream.range(0, count).forEach(i -> { + try { + files.add(writeFile(cachedFileReferences, "fileReference" + i)); + } catch (IOException e) { + throw new RuntimeException(e); + } + }); + return files; + } + + private List<File> writeDownloads(int count) { + List<File> files = new ArrayList<>(); + IntStream.range(0, count).forEach(i -> { + try { + files.add(writeFile(cachedDownloads, "download" + i)); + } catch (IOException e) { + throw new RuntimeException(e); + } + }); + return files; } private void updateLastModifiedTimeStamp(File file, Instant instant) { @@ -55,7 +110,7 @@ public class FileReferencesAndDownloadsMaintainerTest { } private void runMaintainerAndAssertFiles(int fileReferenceCount, int downloadCount) { - cachedFilesMaintainer.run(); + maintainer.run(); File[] fileReferences = cachedFileReferences.listFiles(); assertNotNull(fileReferences); assertEquals(fileReferenceCount, fileReferences.length); diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index 45666355901..4e995e9b392 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -331,7 +331,7 @@ public class Flags { "Takes effect at next host-admin tick"); public static final UnboundBooleanFlag ENABLE_THE_ONE_THAT_SHOULD_NOT_BE_NAMED = defineFeatureFlag( - "enable-the-one-that-should-not-be-named", false, List.of("hmusum"), "2023-05-08", "2023-08-15", + "enable-the-one-that-should-not-be-named", false, List.of("hmusum"), "2023-05-08", "2023-09-15", "Whether to enable the one program that should not be named", "Takes effect at next host-admin tick"); |