diff options
77 files changed, 862 insertions, 668 deletions
diff --git a/bundle-plugin-test/integration-test/pom.xml b/bundle-plugin-test/integration-test/pom.xml index acd075d0365..57ca134ed05 100644 --- a/bundle-plugin-test/integration-test/pom.xml +++ b/bundle-plugin-test/integration-test/pom.xml @@ -58,6 +58,12 @@ </dependency> <dependency> <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>export-packages-lib</artifactId> + <classifier>bundle</classifier> + <version>${project.version}</version> + </dependency> + <dependency> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> <artifactId>non-public-api-usage</artifactId> <classifier>bundle</classifier> <version>${project.version}</version> diff --git a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java index 673d7d8e09e..a9b482377fa 100644 --- a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java +++ b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java @@ -10,12 +10,15 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Arrays; import java.util.Enumeration; +import java.util.List; import java.util.jar.Attributes; import java.util.jar.JarEntry; import java.util.jar.JarFile; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Collectors; import java.util.zip.ZipEntry; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -105,21 +108,38 @@ public class BundleTest { @Test void require_that_manifest_contains_public_api_for_this_bundle_and_embedded_bundles() { - assertEquals("com.yahoo.test,com.yahoo.vespa.defaults", mainAttributes.getValue("X-JDisc-PublicApi-Package")); + var publicApiAttribute = mainAttributes.getValue("X-JDisc-PublicApi-Package"); + assertNotNull(publicApiAttribute); + var publicApi = Arrays.stream(publicApiAttribute.split(",")).collect(Collectors.toSet()); + + var expected = List.of("ai.vespa.lib.public_api", "com.yahoo.lib.public_api", "com.yahoo.test"); + assertEquals(expected.size(), publicApi.size()); + expected.forEach(pkg -> assertTrue(publicApi.contains(pkg), "Public api did not contain %s".formatted(pkg))); } @Test + void require_that_manifest_contains_non_public_api_for_this_bundle_and_embedded_bundles() { + var nonPublicApiAttribute = mainAttributes.getValue("X-JDisc-Non-PublicApi-Export-Package"); + assertNotNull(nonPublicApiAttribute); + var nonPublicApi = Arrays.stream(nonPublicApiAttribute.split(",")).collect(Collectors.toSet()); + + var expected = List.of("ai.vespa.lib.non_public", "com.yahoo.lib.non_public", "com.yahoo.non_public"); + assertEquals(expected.size(), nonPublicApi.size()); + expected.forEach(pkg -> assertTrue(nonPublicApi.contains(pkg), "Non-public api did not contain %s".formatted(pkg))); + } + + @Test void require_that_manifest_contains_bundle_class_path() { String bundleClassPath = mainAttributes.getValue("Bundle-ClassPath"); assertTrue(bundleClassPath.contains(".,")); - Pattern jrtPattern = Pattern.compile("dependencies/defaults" + snapshotOrVersionOrNone); - assertTrue(jrtPattern.matcher(bundleClassPath).find(), "Bundle class path did not contain 'defaults''."); + Pattern jrtPattern = Pattern.compile("dependencies/export-packages-lib" + snapshotOrVersionOrNone); + assertTrue(jrtPattern.matcher(bundleClassPath).find(), "Bundle class path did not contain 'export-packages-lib''."); } @Test void require_that_component_jar_file_contains_compile_artifacts() { - String requiredDep = "dependencies/defaults"; + String requiredDep = "dependencies/export-packages-lib"; Pattern depPattern = Pattern.compile(requiredDep + snapshotOrVersionOrNone); ZipEntry depEntry = null; @@ -133,7 +153,7 @@ public class BundleTest { } } } - assertNotNull(depEntry, "Component jar file did not contain 'defaults' dependency."); + assertNotNull(depEntry, "Component jar file did not contain 'export-packages-lib' dependency."); } diff --git a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java index 42ac99c65e5..9111cd00f82 100644 --- a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java +++ b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java @@ -6,13 +6,13 @@ import org.junit.jupiter.api.Test; import java.io.File; import java.io.IOException; import java.util.Arrays; -import java.util.Set; import java.util.jar.Attributes; import java.util.jar.JarFile; import java.util.stream.Collectors; import static com.yahoo.container.plugin.BundleTest.findBundleJar; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -20,15 +20,13 @@ import static org.junit.jupiter.api.Assertions.assertTrue; */ public class NonPublicApiDetectionTest { - private static Set<String> usedNonPublicApi; + private static Attributes mainAttributes; @BeforeAll public static void setup() { try { File componentJar = findBundleJar("non-public-api-usage"); - Attributes mainAttributes = new JarFile(componentJar).getManifest().getMainAttributes(); - var nonPublicApiAttribute = mainAttributes.getValue("X-JDisc-Non-PublicApi-Import-Package"); - usedNonPublicApi = Arrays.stream(nonPublicApiAttribute.split(",")).collect(Collectors.toSet()); + mainAttributes = new JarFile(componentJar).getManifest().getMainAttributes(); } catch (IOException e) { throw new RuntimeException(e); } @@ -36,9 +34,13 @@ public class NonPublicApiDetectionTest { @Test void usage_of_non_publicApi_packages_is_detected() { + var nonPublicApiAttribute = mainAttributes.getValue("X-JDisc-Non-PublicApi-Import-Package"); + assertNotNull(nonPublicApiAttribute); + var usedNonPublicApi = Arrays.stream(nonPublicApiAttribute.split(",")).collect(Collectors.toSet()); + assertEquals(2, usedNonPublicApi.size()); - assertTrue(usedNonPublicApi.contains("ai.vespa.http")); - assertTrue(usedNonPublicApi.contains("com.yahoo.io")); + assertTrue(usedNonPublicApi.contains("ai.vespa.lib.non_public")); + assertTrue(usedNonPublicApi.contains("com.yahoo.lib.non_public")); } } diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/pom.xml b/bundle-plugin-test/test-bundles/export-packages-lib/pom.xml new file mode 100644 index 00000000000..09eedda0b98 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/pom.xml @@ -0,0 +1,31 @@ +<?xml version="1.0"?> +<!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. --> +<project xmlns="http://maven.apache.org/POM/4.0.0" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 + http://maven.apache.org/xsd/maven-4.0.0.xsd"> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>test-bundles</artifactId> + <version>8-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + <artifactId>export-packages-lib</artifactId> + <version>8-SNAPSHOT</version> + <packaging>container-plugin</packaging> + + <build> + <plugins> + <plugin> + <groupId>com.yahoo.vespa</groupId> + <artifactId>bundle-plugin</artifactId> + <extensions>true</extensions> + <configuration> + <!-- Must be an internal bundle to get manifest header for non-public export packages --> + <bundleType>INTERNAL</bundleType> + </configuration> + </plugin> + </plugins> + </build> +</project> diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/non_public/Foo.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/non_public/Foo.java new file mode 100644 index 00000000000..60c9692cba7 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/non_public/Foo.java @@ -0,0 +1,5 @@ +package ai.vespa.lib.non_public; + +public interface Foo { + +} diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/non_public/package-info.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/non_public/package-info.java new file mode 100644 index 00000000000..18841cf6f37 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/non_public/package-info.java @@ -0,0 +1,5 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +package ai.vespa.lib.non_public; + +import com.yahoo.osgi.annotation.ExportPackage; diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/public_api/Foo.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/public_api/Foo.java new file mode 100644 index 00000000000..cd3b5a2d065 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/public_api/Foo.java @@ -0,0 +1,5 @@ +package ai.vespa.lib.public_api; + +public interface Foo { + +} diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/public_api/package-info.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/public_api/package-info.java new file mode 100644 index 00000000000..c586faf6bb6 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/ai/vespa/lib/public_api/package-info.java @@ -0,0 +1,7 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +@PublicApi +package ai.vespa.lib.public_api; + +import com.yahoo.api.annotations.PublicApi; +import com.yahoo.osgi.annotation.ExportPackage; diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/non_public/Foo.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/non_public/Foo.java new file mode 100644 index 00000000000..3f8c4540d75 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/non_public/Foo.java @@ -0,0 +1,5 @@ +package com.yahoo.lib.non_public; + +public interface Foo { + +} diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/non_public/package-info.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/non_public/package-info.java new file mode 100644 index 00000000000..a3368af200c --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/non_public/package-info.java @@ -0,0 +1,5 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +package com.yahoo.lib.non_public; + +import com.yahoo.osgi.annotation.ExportPackage; diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/public_api/Foo.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/public_api/Foo.java new file mode 100644 index 00000000000..623783d0c01 --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/public_api/Foo.java @@ -0,0 +1,5 @@ +package com.yahoo.lib.public_api; + +public interface Foo { + +} diff --git a/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/public_api/package-info.java b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/public_api/package-info.java new file mode 100644 index 00000000000..b0ff91eb53e --- /dev/null +++ b/bundle-plugin-test/test-bundles/export-packages-lib/src/main/java/com/yahoo/lib/public_api/package-info.java @@ -0,0 +1,7 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +@PublicApi +package com.yahoo.lib.public_api; + +import com.yahoo.api.annotations.PublicApi; +import com.yahoo.osgi.annotation.ExportPackage; diff --git a/bundle-plugin-test/test-bundles/main/pom.xml b/bundle-plugin-test/test-bundles/main/pom.xml index a6cf45947f3..18963343283 100644 --- a/bundle-plugin-test/test-bundles/main/pom.xml +++ b/bundle-plugin-test/test-bundles/main/pom.xml @@ -16,8 +16,8 @@ <packaging>container-plugin</packaging> <dependencies> <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>defaults</artifactId> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>export-packages-lib</artifactId> <version>${project.version}</version> </dependency> <dependency> diff --git a/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/non_public/package-info.java b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/non_public/package-info.java new file mode 100644 index 00000000000..143bb7b5c86 --- /dev/null +++ b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/non_public/package-info.java @@ -0,0 +1,5 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +package com.yahoo.non_public; + +import com.yahoo.osgi.annotation.ExportPackage; diff --git a/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml b/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml index 5386346b8f7..2a621fd97c2 100644 --- a/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml +++ b/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml @@ -17,14 +17,8 @@ <dependencies> <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>defaults</artifactId> - <version>${project.version}</version> - <scope>provided</scope> - </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>vespajlib</artifactId> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>export-packages-lib</artifactId> <version>${project.version}</version> <scope>provided</scope> </dependency> diff --git a/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java b/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java index f2c64661ad6..7bea3252ea5 100644 --- a/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java +++ b/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java @@ -3,13 +3,12 @@ package com.yahoo.test; public class UsingBothPublicApiAndNonPublicApiPackages { - com.yahoo.vespa.defaults.Defaults publicFromDefaults = null; + ai.vespa.lib.non_public.Foo non_public_ai_vespa = null; - com.yahoo.text.BooleanParser publicFromVespajlib = null; + ai.vespa.lib.public_api.Foo public_ai_vespa = null; + com.yahoo.lib.non_public.Foo non_public_com_yahoo = null; - ai.vespa.http.DomainName nonPublic1 = null; - - com.yahoo.io.ByteWriter nonPublic2 = null; + com.yahoo.lib.public_api.Foo public_com_yahoo = null; } diff --git a/bundle-plugin-test/test-bundles/pom.xml b/bundle-plugin-test/test-bundles/pom.xml index 34c6b2e4540..71c0e549be6 100644 --- a/bundle-plugin-test/test-bundles/pom.xml +++ b/bundle-plugin-test/test-bundles/pom.xml @@ -50,6 +50,7 @@ <modules> <module>artifact-version-for-exports</module> <module>artifact-version-for-exports-dep</module> + <module>export-packages-lib</module> <module>non-public-api-usage</module> <module>main</module> </modules> diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java index af6c82023ab..8b32c6c0d0d 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java @@ -12,9 +12,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.jar.Manifest; -import java.util.stream.Collectors; /** * Static utilities for analyzing jar files. @@ -45,18 +43,18 @@ public class AnalyzeBundle { } } - public static List<String> publicApiPackagesAggregated(Collection<File> jarFiles) { + public static List<String> nonPublicApiPackagesAggregated(Collection<File> jarFiles) { return jarFiles.stream() - .map(AnalyzeBundle::publicApiPackages) + .map(AnalyzeBundle::nonPublicApiPackages) .flatMap(List::stream) .distinct() .toList(); } - static List<String> publicApiPackages(File jarFile) { + private static List<String> nonPublicApiPackages(File jarFile) { var manifest = getOsgiManifest(jarFile); if (manifest == null) return Collections.emptyList(); - return getMainAttributeValue(manifest, "X-JDisc-PublicApi-Package") + return getMainAttributeValue(manifest, "X-JDisc-Non-PublicApi-Export-Package") .map(s -> Arrays.asList(s.split(","))) .orElseGet(ArrayList::new); } diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Analyze.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Analyze.java index a93f8cb87d5..4fd8e936f3d 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Analyze.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Analyze.java @@ -21,7 +21,7 @@ import java.util.Optional; */ public class Analyze { - public static ClassFileMetaData analyzeClass(File classFile) { + static ClassFileMetaData analyzeClass(File classFile) { return analyzeClass(classFile, null); } @@ -52,16 +52,12 @@ public class Analyze { } static Optional<String> getClassName(Type aType) { - switch (aType.getSort()) { - case Type.ARRAY: - return getClassName(aType.getElementType()); - case Type.OBJECT: - return Optional.of(aType.getClassName()); - case Type.METHOD: - return getClassName(aType.getReturnType()); - default: - return Optional.empty(); - } + return switch (aType.getSort()) { + case Type.ARRAY -> getClassName(aType.getElementType()); + case Type.OBJECT -> Optional.of(aType.getClassName()); + case Type.METHOD -> getClassName(aType.getReturnType()); + default -> Optional.empty(); + }; } static AnnotationVisitor visitAnnotationDefault(ImportCollector collector) { diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/AnalyzeClassVisitor.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/AnalyzeClassVisitor.java index e57af606b3a..310527e9254 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/AnalyzeClassVisitor.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/AnalyzeClassVisitor.java @@ -120,18 +120,10 @@ class AnalyzeClassVisitor extends ClassVisitor implements ImportCollector { public void visit(String name, Object value) { if (name != null) { switch (name) { - case "major": - major = (int) value; - break; - case "minor": - minor = (int) value; - break; - case "micro": - micro = (int) value; - break; - case "qualifier": - qualifier = (String) value; - break; + case "major" -> major = (int) value; + case "minor" -> minor = (int) value; + case "micro" -> micro = (int) value; + case "qualifier" -> qualifier = (String) value; } } } diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageInfo.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageInfo.java index c19320b8e98..7b665d67931 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageInfo.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageInfo.java @@ -3,12 +3,16 @@ package com.yahoo.container.plugin.classanalysis; import java.util.Optional; /** - * The package + * Info about a Java package deduced from class analysis. * * @author gjoranv */ record PackageInfo(String name, Optional<ExportPackageAnnotation> exportPackage, boolean isPublicApi) { + /** + * Returns this if it has an ExportPackage annotation, otherwise returns the other. + * Used to combine objects, where this should take precedence over the other. + */ PackageInfo hasExportPackageOrElse(PackageInfo other) { return exportPackage().isPresent() ? this : other; } diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java index 51fba228b41..699736195cf 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/PackageTally.java @@ -8,7 +8,6 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -49,6 +48,14 @@ public class PackageTally { .collect(Collectors.toSet()); } + public Set<String> nonPublicApiExportedPackages() { + return definedPackages.values().stream() + .filter(pkgInfo -> pkgInfo.exportPackage().isPresent()) + .filter(pkgInfo -> ! pkgInfo.isPublicApi()) + .map(PackageInfo::name) + .collect(Collectors.toSet()); + } + /** * Returns the set of packages that is referenced from this tally, but not included in the given set of available packages. * diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java index 48a128c2f0d..a9f95f2b2f2 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java @@ -7,7 +7,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; /** * Utility methods related to packages. @@ -37,19 +36,17 @@ public class Packages { } /** - * Returns the imported Vespa packages that don't exist in the given set of allowed packages. + * Returns the imported packages that exist in the given set of disallowed packages. */ - public static List<String> disallowedVespaImports(Map<String, ImportPackages.Import> imports, List<String> allowed) { + public static List<String> disallowedImports(Map<String, ImportPackages.Import> imports, List<String> disallowed) { if (imports == null || imports.isEmpty()) return List.of(); - var publicApi = allowed == null ? Set.of() : new HashSet<>(allowed); + var importedSet = new HashSet<>(imports.keySet()); + var disallowedSet = disallowed == null ? Set.of() : new HashSet<>(disallowed); - Set<String> yahooImports = imports.keySet().stream() - .filter(pkg -> pkg.startsWith("com.yahoo") || pkg.startsWith("ai.vespa.")) - .collect(Collectors.toSet()); - - List<String> disallowedImports = yahooImports.stream().collect(Collectors.groupingBy(publicApi::contains)).get(false); - return disallowedImports == null ? List.of() : disallowedImports; + return importedSet.stream() + .filter(disallowedSet::contains) + .toList(); } public static PackageMetaData analyzePackages(Set<ClassFileMetaData> allClasses) { diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java index d217273e42b..ff802668427 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java @@ -26,8 +26,8 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import static com.yahoo.container.plugin.bundle.AnalyzeBundle.exportedPackagesAggregated; -import static com.yahoo.container.plugin.bundle.AnalyzeBundle.publicApiPackagesAggregated; -import static com.yahoo.container.plugin.classanalysis.Packages.disallowedVespaImports; +import static com.yahoo.container.plugin.bundle.AnalyzeBundle.nonPublicApiPackagesAggregated; +import static com.yahoo.container.plugin.classanalysis.Packages.disallowedImports; import static com.yahoo.container.plugin.osgi.ExportPackages.exportsByPackageName; import static com.yahoo.container.plugin.osgi.ImportPackages.calculateImports; import static com.yahoo.container.plugin.util.Files.allDescendantFiles; @@ -95,7 +95,7 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo { List<Artifact> providedJarArtifacts = artifactSet.getJarArtifactsProvided(); List<File> providedJarFiles = providedJarArtifacts.stream().map(Artifact::getFile).toList(); List<Export> exportedPackagesFromProvidedJars = exportedPackagesAggregated(providedJarFiles); - List<String> publicApiPackagesFromProvidedJars = publicApiPackagesAggregated(providedJarFiles); + List<String> nonPublicApiPackagesFromProvidedJars = nonPublicApiPackagesAggregated(providedJarFiles); // Packages from Export-Package/PublicApi headers in provided scoped jars Set<String> exportedPackagesFromProvidedDeps = ExportPackages.packageNames(exportedPackagesFromProvidedJars); @@ -125,14 +125,15 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo { includedPackages.definedPackages(), exportsByPackageName(exportedPackagesFromProvidedJars)); - List<String> nonPublicApiUsed = disallowedVespaImports(calculatedImports, publicApiPackagesFromProvidedJars); + List<String> nonPublicApiUsed = disallowedImports(calculatedImports, nonPublicApiPackagesFromProvidedJars); logNonPublicApiUsage(nonPublicApiUsed); Map<String, String> manifestContent = generateManifestContent(artifactSet.getJarArtifactsToInclude(), calculatedImports, includedPackages); - addAdditionalManifestProperties(manifestContent, includedPackages); + addAdditionalManifestProperties(manifestContent); + addManifestPropertiesForInternalBundles(manifestContent, includedPackages); addManifestPropertiesForUserBundles(manifestContent, nonPublicApiUsed); - createManifestFile(Paths.get(project.getBuild().getOutputDirectory()), manifestContent); + createManifestFile(Paths.get(project.getBuild().getOutputDirectory()), manifestContent); } catch (Exception e) { throw new MojoExecutionException("Failed generating osgi manifest", e); } @@ -151,8 +152,7 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo { return isVespaInternalGroupId(project.getGroupId()) ? BundleType.INTERNAL : BundleType.USER; } - private void addAdditionalManifestProperties(Map<String, String> manifestContent, PackageTally includedPackages) { - addIfNotEmpty(manifestContent, "X-JDisc-PublicApi-Package", publicApi(includedPackages)); + private void addAdditionalManifestProperties(Map<String, String> manifestContent) { addIfNotEmpty(manifestContent, "Bundle-Activator", bundleActivator); addIfNotEmpty(manifestContent, "X-JDisc-Privileged-Activator", jdiscPrivilegedActivator); addIfNotEmpty(manifestContent, "Main-Class", mainClass); @@ -161,6 +161,15 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo { addIfNotEmpty(manifestContent, "WebInfUrl", webInfUrl); } + private void addManifestPropertiesForInternalBundles(Map<String, String> manifestContent, PackageTally includedPackages) { + if (effectiveBundleType() == BundleType.USER) return; + + // TODO: this attribute is not necessary, remove? + addIfNotEmpty(manifestContent, "X-JDisc-PublicApi-Package", publicApi(includedPackages)); + + addIfNotEmpty(manifestContent, "X-JDisc-Non-PublicApi-Export-Package", nonPublicApi(includedPackages)); + } + private void addManifestPropertiesForUserBundles(Map<String, String> manifestContent, List<String> nonPublicApiUsed) { if (effectiveBundleType() != BundleType.USER) return; addIfNotEmpty(manifestContent, "X-JDisc-Non-PublicApi-Import-Package", String.join(",", nonPublicApiUsed)); @@ -175,6 +184,10 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo { return tally.publicApiPackages().stream().sorted().collect(Collectors.joining(",")); } + private static String nonPublicApi(PackageTally tally) { + return tally.nonPublicApiExportedPackages().stream().sorted().collect(Collectors.joining(",")); + } + private void logDebugPackageSets(List<Export> exportedPackagesFromProvidedJars, PackageTally includedPackages) { if (getLog().isDebugEnabled()) { getLog().debug("Referenced packages = " + includedPackages.referencedPackages()); diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java index a4be547fe70..b6f934c8824 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java @@ -344,7 +344,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { int deployableHashCode() { List<DeploymentSpec.DeclaredZone> zones = zones().stream().filter(zone -> zone.concerns(prod)).toList(); - Object[] toHash = new Object[zones.size() + 6]; + Object[] toHash = new Object[zones.size() + 7]; int i = 0; toHash[i++] = name; toHash[i++] = endpoints; @@ -352,8 +352,9 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { toHash[i++] = globalServiceId; toHash[i++] = tags; toHash[i++] = bcp; + toHash[i++] = cloudAccounts; for (DeploymentSpec.DeclaredZone zone : zones) - toHash[i++] = Objects.hash(zone, zone.athenzService()); + toHash[i++] = Objects.hash(zone, zone.athenzService(), zone.cloudAccounts()); return Arrays.hashCode(toHash); } diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java index f355a61fa8a..797be652ebc 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java @@ -359,12 +359,13 @@ public class DeploymentSpec { /** Computes a hash of all fields that influence what is deployed with this spec, i.e., not orchestration. */ public int deployableHashCode() { - Object[] toHash = new Object[instances().size() + 4]; + Object[] toHash = new Object[instances().size() + 5]; int i = 0; toHash[i++] = majorVersion; toHash[i++] = athenzDomain; toHash[i++] = athenzService; toHash[i++] = endpoints; + toHash[i++] = cloudAccounts; for (DeploymentInstanceSpec instance : instances()) toHash[i++] = instance.deployableHashCode(); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/StreamingValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/StreamingValidator.java index ad126cfa22b..6008536db0b 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/StreamingValidator.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/StreamingValidator.java @@ -65,7 +65,17 @@ public class StreamingValidator extends Validator { // attribute indexing ourselves (IntegerIndex2Attribute) if (sd.getDataType() instanceof NumericDataType) return; // Tensor fields are only searchable via nearest neighbor search, and match semantics are irrelevant. - if (sd.getDataType() instanceof TensorDataType) return; + if (sd.getDataType() instanceof TensorDataType) { + for (var fieldAttribute : sd.getAttributes().values()) { + if (fieldAttribute.hnswIndexParams().isPresent()) { + logger.logApplicationPackage(Level.WARNING, + "For streaming search cluster '" + sc.getClusterName() + + "', SD field '" + sd.getName() + + "': hnsw index is not relevant and not supported, ignoring setting"); + } + } + return; + } logger.logApplicationPackage(Level.WARNING, "For streaming search cluster '" + sc.getClusterName() + "', SD field '" + sd.getName() + "': 'attribute' has same match semantics as 'index'."); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/DataplaneProxy.java b/config-model/src/main/java/com/yahoo/vespa/model/container/DataplaneProxy.java index fe7d9581e46..3349aee9f2a 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/DataplaneProxy.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/DataplaneProxy.java @@ -10,12 +10,16 @@ public class DataplaneProxy extends SimpleComponent implements DataplaneProxyCon private final Integer port; private final String serverCertificate; private final String serverKey; + private final String mTlsEndpoint; + private final String tokenEndpoint; - public DataplaneProxy(Integer port, String serverCertificate, String serverKey) { + public DataplaneProxy(Integer port, String serverCertificate, String serverKey, String mTlsEndpoint, String tokenEndpoint) { super(DataplaneProxyConfigurator.class.getName()); this.port = port; this.serverCertificate = serverCertificate; this.serverKey = serverKey; + this.mTlsEndpoint = mTlsEndpoint; + this.tokenEndpoint = tokenEndpoint; } @Override @@ -23,6 +27,8 @@ public class DataplaneProxy extends SimpleComponent implements DataplaneProxyCon builder.port(port); builder.serverCertificate(serverCertificate); builder.serverKey(serverKey); + builder.mTlsEndpoint(mTlsEndpoint); + builder.tokenEndpoint(tokenEndpoint); } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java index ce999603439..4923c41224e 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java @@ -4,11 +4,18 @@ package com.yahoo.vespa.model.container.search; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.io.IOUtils; import com.yahoo.io.reader.NamedReader; +import com.yahoo.language.simple.SimpleLinguistics; +import com.yahoo.prelude.semantics.RuleBase; +import com.yahoo.prelude.semantics.RuleImporter; +import com.yahoo.prelude.semantics.SemanticRulesConfig; +import com.yahoo.prelude.semantics.parser.ParseException; import java.io.File; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; -import java.util.stream.Collectors; +import java.util.Map; /** * Reads the semantic rules from the application package by delegating to SemanticRules. @@ -18,16 +25,22 @@ import java.util.stream.Collectors; // TODO: Move into SemanticRules public class SemanticRuleBuilder { - /** Build the set of semantic rules for an application package */ + /** Builds the semantic rules for an application package and validates them */ public SemanticRules build(ApplicationPackage applicationPackage) { - List<NamedReader> ruleBaseFiles = null; + var ruleFiles = applicationPackage.getFiles(ApplicationPackage.RULES_DIR, "sr"); + var rules = new SemanticRules(ruleFiles.stream().map(this::toRuleBaseConfigView).toList()); + + // Create config to make sure rules are valid, config is validated in call to toMap() below + var builder = new SemanticRulesConfig.Builder(); + rules.getConfig(builder); + SemanticRulesConfig config = builder.build(); try { - ruleBaseFiles = applicationPackage.getFiles(ApplicationPackage.RULES_DIR, "sr"); - return new SemanticRules(ruleBaseFiles.stream().map(this::toRuleBaseConfigView).toList()); - } - finally { - NamedReader.closeAll(ruleBaseFiles); + toMap(config); // validates config + ensureZeroOrOneDefaultRule(config); + } catch (ParseException | IOException e) { + throw new RuntimeException(e); } + return rules; } private SemanticRules.RuleBase toRuleBaseConfigView(NamedReader reader) { @@ -43,7 +56,32 @@ public class SemanticRuleBuilder { private String toName(String fileName) { String shortName = new File(fileName).getName(); - return shortName.substring(0, shortName.length()-".sr".length()); + return shortName.substring(0, shortName.length() - ".sr".length()); + } + + private void ensureZeroOrOneDefaultRule(SemanticRulesConfig config) { + String defaultName = null; + for (SemanticRulesConfig.Rulebase ruleBase : config.rulebase()) { + if (defaultName != null && ruleBase.isdefault()) { + List<String> defaultRules = new ArrayList<>(List.of(defaultName, ruleBase.name())); + defaultRules.sort(String::compareTo); + throw new IllegalArgumentException("Rules " + defaultRules + " are both marked as the default rule, there can only be one"); + } + if (ruleBase.isdefault()) + defaultName = ruleBase.name(); + } + } + + static Map<String, RuleBase> toMap(SemanticRulesConfig config) throws ParseException, IOException { + RuleImporter ruleImporter = new RuleImporter(config, true, new SimpleLinguistics()); + Map<String, RuleBase> ruleBaseMap = new HashMap<>(); + for (SemanticRulesConfig.Rulebase ruleBaseConfig : config.rulebase()) { + RuleBase ruleBase = ruleImporter.importConfig(ruleBaseConfig); + if (ruleBaseConfig.isdefault()) + ruleBase.setDefault(true); + ruleBaseMap.put(ruleBase.getName(), ruleBase); + } + return ruleBaseMap; } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java index 46c8577c6e8..0083fc6b886 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java @@ -6,14 +6,14 @@ import java.io.Serializable; import java.util.List; /** - * Returns the semantic rules config form a set of rule bases. + * Returns the semantic rules config from a set of rule bases. * Owned by a container cluster * * @author bratseth */ public class SemanticRules implements Serializable, SemanticRulesConfig.Producer { - private List<RuleBase> ruleBases; + private final List<RuleBase> ruleBases; public SemanticRules(List<RuleBase> ruleBases) { this.ruleBases = ruleBases; @@ -46,7 +46,6 @@ public class SemanticRules implements Serializable, SemanticRulesConfig.Producer return ruleBaseBuilder; } - } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index bcebf1a9fdd..f795dc4bd93 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -602,10 +602,19 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { cluster.addSimpleComponent(DataplaneProxyCredentials.class); cluster.addSimpleComponent(DataplaneProxyService.class); + var mTlsEndpoint = cluster.endpoints() + .stream() + .filter(endpoint -> endpoint.scope().equals(ApplicationClusterEndpoint.Scope.zone)) + .findFirst() + .map(endpoint -> endpoint.dnsName().value()) + .orElseThrow(); + var dataplaneProxy = new DataplaneProxy( getDataplanePort(deployState), endpointCertificateSecrets.certificate(), - endpointCertificateSecrets.key()); + endpointCertificateSecrets.key(), + mTlsEndpoint, + "token." + mTlsEndpoint); cluster.addComponent(dataplaneProxy); } connectorFactory = authorizeClient diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/StreamingValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/StreamingValidatorTest.java index fd1e6be27fd..82bd96d32d7 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/StreamingValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/StreamingValidatorTest.java @@ -1,9 +1,20 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.model.application.validation; +import com.yahoo.config.application.api.DeployLogger; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.schema.derived.TestableDeployLogger; +import com.yahoo.vespa.model.VespaModel; +import com.yahoo.vespa.model.content.utils.ApplicationPackageBuilder; +import com.yahoo.vespa.model.content.utils.ContentClusterBuilder; +import com.yahoo.vespa.model.content.utils.DocType; +import com.yahoo.vespa.model.content.utils.SchemaBuilder; import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithFilePkg; import org.junit.jupiter.api.Test; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -21,4 +32,31 @@ public class StreamingValidatorTest { assertTrue(exception.getMessage().contains("For streaming search cluster 'content.ad': Attribute 'campaign_ref' has type 'Reference<campaign>'. " + "Document references and imported fields are not allowed in streaming search.")); } + + @Test + void tensor_field_without_index_gives_no_warning() { + var logger = new TestableDeployLogger(); + var model = createModel(logger, "field nn type tensor(x[2]) { indexing: attribute | summary\n" + + "attribute { distance-metric: euclidean } }"); + assertTrue(logger.warnings.isEmpty()); + } + + @Test + void tensor_field_with_index_triggers_warning_in_streaming_search() { + var logger = new TestableDeployLogger(); + var model = createModel(logger, "field nn type tensor(x[2]) { indexing: attribute | index | summary\n" + + "attribute { distance-metric: euclidean } }"); + assertEquals(1, logger.warnings.size()); + assertEquals("For streaming search cluster 'content.test', SD field 'nn': hnsw index is not relevant and not supported, ignoring setting", + logger.warnings.get(0)); + } + + private static VespaModel createModel(DeployLogger logger, String sdContent) { + var builder = new DeployState.Builder(); + builder.deployLogger(logger); + return new ApplicationPackageBuilder() + .addCluster(new ContentClusterBuilder().name("content").docTypes(List.of(DocType.streaming("test")))) + .addSchemas(new SchemaBuilder().name("test").content(sdContent).build()) + .buildCreator().create(builder); + } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java index d9e2ae59ef6..da6e055724a 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java @@ -2,52 +2,66 @@ package com.yahoo.vespa.model.container.search; import com.yahoo.config.model.application.provider.FilesApplicationPackage; -import com.yahoo.language.simple.SimpleLinguistics; import com.yahoo.prelude.semantics.RuleBase; -import com.yahoo.prelude.semantics.RuleImporter; import com.yahoo.prelude.semantics.SemanticRulesConfig; import com.yahoo.prelude.semantics.parser.ParseException; import org.junit.jupiter.api.Test; -import static org.junit.jupiter.api.Assertions.*; - import java.io.File; import java.io.IOException; -import java.util.HashMap; import java.util.Map; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + /** * @author bratseth */ public class SemanticRulesTest { - private final static String root = "src/test/java/com/yahoo/vespa/model/container/search/semanticrules"; + private static final String basePath = "src/test/java/com/yahoo/vespa/model/container/search/"; + private static final String root = basePath + "semanticrules"; + private static final String rootWithErrors = basePath + "semanticrules_with_errors"; + private static final String rootWithDuplicateDefault = basePath + "semanticrules_with_duplicate_default_rule"; @Test - void semanticRulesTest() throws ParseException, IOException { + void semanticRulesTest() throws ParseException, IOException { SemanticRuleBuilder ruleBuilder = new SemanticRuleBuilder(); SemanticRules rules = ruleBuilder.build(FilesApplicationPackage.fromFile(new File(root))); SemanticRulesConfig.Builder configBuilder = new SemanticRulesConfig.Builder(); rules.getConfig(configBuilder); SemanticRulesConfig config = new SemanticRulesConfig(configBuilder); - Map<String, RuleBase> ruleBases = toMap(config); + Map<String, RuleBase> ruleBases = SemanticRuleBuilder.toMap(config); assertEquals(2, ruleBases.size()); assertTrue(ruleBases.containsKey("common")); assertTrue(ruleBases.containsKey("other")); assertFalse(ruleBases.get("common").isDefault()); assertTrue(ruleBases.get("other").isDefault()); + assertTrue(ruleBases.get("other").includes("common")); + assertNotNull(ruleBases.get("other").getCondition("stopword")); } - private static Map<String, RuleBase> toMap(SemanticRulesConfig config) throws ParseException, IOException { - RuleImporter ruleImporter = new RuleImporter(config, new SimpleLinguistics()); - Map<String, RuleBase> ruleBaseMap = new HashMap<>(); - for (SemanticRulesConfig.Rulebase ruleBaseConfig : config.rulebase()) { - RuleBase ruleBase = ruleImporter.importConfig(ruleBaseConfig); - if (ruleBaseConfig.isdefault()) - ruleBase.setDefault(true); - ruleBaseMap.put(ruleBase.getName(), ruleBase); + @Test + void rulesWithErrors() { + try { + new SemanticRuleBuilder().build(FilesApplicationPackage.fromFile(new File(rootWithErrors))); + fail("should fail with exception"); + } catch (Exception e) { + assertEquals("com.yahoo.prelude.semantics.parser.ParseException: Could not parse rule 'invalid'", e.getMessage()); + } + } + + @Test + void rulesWithDuplicateDefault() { + try { + new SemanticRuleBuilder().build(FilesApplicationPackage.fromFile(new File(rootWithDuplicateDefault))); + fail("should fail with exception"); + } catch (Exception e) { + assertEquals("Rules [one, other] are both marked as the default rule, there can only be one", e.getMessage()); } - return ruleBaseMap; } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr index 3833641a56c..92b279aada4 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr @@ -1,4 +1,5 @@ # Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@automata(etc/vespa/fsa/stopwords.fsa) ## Some test rules # Spelling correction diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr new file mode 100644 index 00000000000..4f2271e91ba --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr @@ -0,0 +1,5 @@ +# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@default + +# Spelling correction +bahc -> bach; diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr new file mode 100644 index 00000000000..29f7e85967f --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr @@ -0,0 +1,5 @@ +# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@default + +# Spelling correction +list-> liszt; diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr new file mode 100644 index 00000000000..9d89cab7e31 --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr @@ -0,0 +1,7 @@ +# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +# Stopwords +[stopword] -> ; +[stopword] :- and, or, the, what, why, how; + +# Synonyms, with wrong character at end of line +[bill] :- Bill, bill, William: diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java index 87793f526bd..6284ecad33e 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java @@ -110,8 +110,8 @@ public class SessionZooKeeperClient { Optional<byte[]> data = curator.getData(sessionStatusPath); return data.map(d -> Session.Status.parse(Utf8.toString(d))).orElse(Session.Status.UNKNOWN); } catch (Exception e) { - log.log(Level.INFO, "Failed to read session status at " + sessionStatusPath.getAbsolute() + - ", will assume session has been removed: ", e); + log.log(Level.INFO, "Failed to read session status from " + sessionStatusPath.getAbsolute() + + ", returning session status 'unknown'"); return Session.Status.UNKNOWN; } } diff --git a/container-search/src/main/java/com/yahoo/prelude/semantics/RuleBase.java b/container-search/src/main/java/com/yahoo/prelude/semantics/RuleBase.java index 5a168d42779..ac98cb54cf4 100644 --- a/container-search/src/main/java/com/yahoo/prelude/semantics/RuleBase.java +++ b/container-search/src/main/java/com/yahoo/prelude/semantics/RuleBase.java @@ -114,7 +114,7 @@ public class RuleBase { * of the given rule base - it can not be subsequently used for any purpose * (including accessing).</p> * - * <p>Each rule base will only be included by the first include directive enountered + * <p>Each rule base will only be included by the first include directive encountered * for that rule base.</p> */ public void include(RuleBase include) { diff --git a/container-search/src/main/java/com/yahoo/prelude/semantics/RuleImporter.java b/container-search/src/main/java/com/yahoo/prelude/semantics/RuleImporter.java index 6e7286cb8dc..40911f793c8 100644 --- a/container-search/src/main/java/com/yahoo/prelude/semantics/RuleImporter.java +++ b/container-search/src/main/java/com/yahoo/prelude/semantics/RuleImporter.java @@ -1,18 +1,16 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.prelude.semantics; +import com.yahoo.io.IOUtils; +import com.yahoo.language.Linguistics; +import com.yahoo.prelude.semantics.parser.ParseException; +import com.yahoo.prelude.semantics.parser.SemanticsParser; + import java.io.BufferedReader; import java.io.File; import java.io.IOException; import java.io.Reader; import java.io.StringReader; -import java.util.Arrays; -import java.util.List; - -import com.yahoo.io.IOUtils; -import com.yahoo.language.Linguistics; -import com.yahoo.prelude.semantics.parser.ParseException; -import com.yahoo.prelude.semantics.parser.SemanticsParser; /** * Imports rule bases from various sources. @@ -22,10 +20,10 @@ import com.yahoo.prelude.semantics.parser.SemanticsParser; // Uses the JavaCC-generated parser to read rule bases. // This is an intermediate between the parser and the rule base being loaded // on implementation of some directives, for example, it knows where to find -// rule bases included into others, while neither the rule base or the parser knows. +// rule bases included into others, while neither the rule base nor the parser knows. public class RuleImporter { - /** If this is set, imported rule bases are looked up in this config otherwise, they are looked up as files. */ + /** If this is set, imported rule bases are looked up in this config, otherwise they are looked up as files. */ private final SemanticRulesConfig config; /** Ignore requests to read automata files. Useful to validate rule bases without having automatas present. */ @@ -34,7 +32,7 @@ public class RuleImporter { /** Ignore requests to include files. Useful to validate rule bases one by one in config. */ private final boolean ignoreIncludes; - private Linguistics linguistics; + private final Linguistics linguistics; /** Create a rule importer which will read from file */ public RuleImporter(Linguistics linguistics) { @@ -108,22 +106,6 @@ public class RuleImporter { } } - /** Imports all the rule files (files ending by "sr") in the given directory */ - public List<RuleBase> importDir(String ruleBaseDir) throws IOException, ParseException { - File ruleBaseDirFile = new File(ruleBaseDir); - if ( ! ruleBaseDirFile.exists()) - throw new IOException("Rule base dir '" + ruleBaseDirFile.getAbsolutePath() + "' does not exist"); - File[] files = ruleBaseDirFile.listFiles(); - Arrays.sort(files); - List<RuleBase> ruleBases = new java.util.ArrayList<>(); - for (File file : files) { - if ( ! file.getName().endsWith(".sr")) continue; - RuleBase base = importFile(file.getAbsolutePath()); - ruleBases.add(base); - } - return ruleBases; - } - /** Read and include a rule base in another */ public void include(String ruleBaseName, RuleBase ruleBase) throws java.io.IOException, ParseException { if (ignoreIncludes) return; @@ -137,7 +119,7 @@ public class RuleImporter { ruleBase.include(include); } - /** Returns an unitialized rule base */ + /** Returns an uninitialized rule base */ private RuleBase privateImportFromDirectory(String ruleBaseName, RuleBase ruleBase) throws IOException, ParseException { String includeDir = new File(ruleBase.getSource()).getParentFile().getAbsolutePath(); if (!ruleBaseName.endsWith(".sr")) @@ -148,7 +130,7 @@ public class RuleImporter { return privateImportFile(importFile.getPath(), null); } - /** Returns an unitialized rule base */ + /** Returns an uninitialized rule base */ private RuleBase privateImportFromConfig(String ruleBaseName) throws ParseException { SemanticRulesConfig.Rulebase ruleBaseConfig = findRuleBaseConfig(config,ruleBaseName); if (ruleBaseConfig == null) @@ -159,10 +141,9 @@ public class RuleImporter { } private SemanticRulesConfig.Rulebase findRuleBaseConfig(SemanticRulesConfig config, String ruleBaseName) { - for (Object aRulebase : config.rulebase()) { - SemanticRulesConfig.Rulebase ruleBaseConfig = (SemanticRulesConfig.Rulebase)aRulebase; - if (ruleBaseConfig.name().equals(ruleBaseName)) - return ruleBaseConfig; + for (SemanticRulesConfig.Rulebase ruleBase : config.rulebase()) { + if (ruleBase.name().equals(ruleBaseName)) + return ruleBase; } return null; } @@ -180,39 +161,28 @@ public class RuleImporter { return fileName.substring(0, lastDotIndex); } - public RuleBase importString(String string, String automataFile) throws IOException, ParseException { + public RuleBase importString(String string, String automataFile) throws ParseException { return importString(string, automataFile, null, null); } - public RuleBase importString(String string, String automataFile, String sourceName) throws IOException, ParseException { - return importString(string, automataFile, sourceName, null); - } - - public RuleBase importString(String string, String automataFile, RuleBase ruleBase) throws IOException, ParseException { - return importString(string, automataFile, null, ruleBase); - } - - public RuleBase importString(String string, String automataFile, String sourceName, RuleBase ruleBase) throws IOException, ParseException { + public RuleBase importString(String string, String automataFile, String sourceName, RuleBase ruleBase) throws ParseException { return importFromReader(new StringReader(string), sourceName, automataFile, ruleBase); } - public RuleBase importConfig(SemanticRulesConfig.Rulebase ruleBaseConfig) throws IOException, ParseException { + public RuleBase importConfig(SemanticRulesConfig.Rulebase ruleBaseConfig) throws ParseException { RuleBase ruleBase = privateImportConfig(ruleBaseConfig); ruleBase.initialize(); return ruleBase; } - /** Imports an unitialized rule base */ + /** Imports an uninitialized rule base */ public RuleBase privateImportConfig(SemanticRulesConfig.Rulebase ruleBaseConfig) throws ParseException { if (config == null) throw new IllegalStateException("Must initialize with config if importing from config"); RuleBase ruleBase = new RuleBase(ruleBaseConfig.name()); return privateImportFromReader(new StringReader(ruleBaseConfig.rules()), - "semantic-rules.cfg", - ruleBaseConfig.automata(),ruleBase); - } - - public RuleBase importFromReader(Reader reader, String sourceInfo, String automataFile) throws ParseException { - return importFromReader(reader, sourceInfo, automataFile, null); + ruleBaseConfig.name(), + ruleBaseConfig.automata(), + ruleBase); } /** @@ -230,27 +200,28 @@ public class RuleImporter { return ruleBase; } - /** Returns an unitialized rule base */ - public RuleBase privateImportFromReader(Reader reader, String sourceName, String automataFile, RuleBase ruleBase) throws ParseException { + /** Returns an uninitialized rule base */ + public RuleBase privateImportFromReader(Reader reader, String inputSourceName, String automataFile, RuleBase ruleBase) throws ParseException { + var sourceName = (inputSourceName == null ? "anonymous" : inputSourceName); try { if (ruleBase == null) - ruleBase = new RuleBase(sourceName == null ? "anonymous" : sourceName); + ruleBase = new RuleBase(sourceName); ruleBase.setSource(sourceName.replace('\\', '/')); new SemanticsParser(reader, linguistics).semanticRules(ruleBase, this); if (automataFile != null && !automataFile.isEmpty()) ruleBase.setAutomataFile(automataFile.replace('\\', '/')); return ruleBase; } catch (Throwable t) { // also catches token mgr errors - ParseException p = new ParseException("Could not parse '" + shortenPath(sourceName) + "'"); + ParseException p = new ParseException("Could not parse rule '" + shortenPath(sourceName) + "'"); p.initCause(t); throw p; } } /** - * Snips what's in from of rules/ if "rules/" is present in the string + * Snips what's in front of rules/ if "rules/" is present in the string * to avoid displaying details about where application content is copied - * (if rules/ is present, these rules are read from an applicatino package) + * (if rules/ is present, these rules are read from an application package) */ private static String shortenPath(String path) { int rulesIndex = path.indexOf("rules/"); diff --git a/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def b/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def index 71fb907ffe2..d8e521ed940 100644 --- a/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def +++ b/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def @@ -3,6 +3,7 @@ namespace=prelude.semantics # Whether we should use these rule bases in pre-Vespa 2.2 compatibility mode +# TODO: Unused, remove in Vespa 9 compatibility bool default=false # The name of a rule base diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/ConfigurationTestCase.java b/container-search/src/test/java/com/yahoo/prelude/semantics/test/ConfigurationTestCase.java index 569ae569f18..ab711eada0f 100644 --- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/ConfigurationTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/semantics/test/ConfigurationTestCase.java @@ -2,14 +2,12 @@ package com.yahoo.prelude.semantics.test; import com.yahoo.component.chain.Chain; -import com.yahoo.config.subscription.ConfigGetter; import com.yahoo.language.simple.SimpleLinguistics; import com.yahoo.prelude.semantics.RuleBase; import com.yahoo.prelude.semantics.RuleBaseException; import com.yahoo.prelude.semantics.SemanticRulesConfig; import com.yahoo.prelude.semantics.SemanticSearcher; import com.yahoo.search.Query; -import com.yahoo.search.Result; import com.yahoo.search.Searcher; import com.yahoo.search.searchchain.Execution; import com.yahoo.search.test.QueryTestCase; @@ -25,17 +23,12 @@ import static org.junit.jupiter.api.Assertions.*; * * @author bratseth */ -@SuppressWarnings("deprecation") public class ConfigurationTestCase { - private static final String root="src/test/java/com/yahoo/prelude/semantics/test/rulebases/"; - private static final SemanticSearcher searcher; - private static final SemanticRulesConfig semanticRulesConfig; static { - semanticRulesConfig = new ConfigGetter<>(SemanticRulesConfig.class).getConfig("file:" + root + "semantic-rules.cfg"); - searcher = new SemanticSearcher(semanticRulesConfig, new SimpleLinguistics()); + searcher = new SemanticSearcher(config(), new SimpleLinguistics()); } protected void assertSemantics(String result, String input, String baseName) { @@ -55,7 +48,7 @@ public class ConfigurationTestCase { RuleBase parent = searcher.getRuleBase("parent"); assertNotNull(parent); assertEquals("parent", parent.getName()); - assertEquals("semantic-rules.cfg", parent.getSource()); + assertEquals("parent", parent.getSource()); } @Test @@ -119,10 +112,10 @@ public class ConfigurationTestCase { assertSemantics("WEAKAND(100) skoda car", "skoda cars", "parent"); } - private Result doSearch(Searcher searcher, Query query, int offset, int hits) { + private void doSearch(Searcher searcher, Query query, int offset, int hits) { query.setOffset(offset); query.setHits(hits); - return createExecution(searcher).search(query); + createExecution(searcher).search(query); } private Execution createExecution(Searcher searcher) { @@ -135,4 +128,29 @@ public class ConfigurationTestCase { return new Chain<>(searchers); } + private static SemanticRulesConfig config() { + + // Create config to make sure rules are valid, config is validated in call to toMap() below + var builder = new SemanticRulesConfig.Builder(); + + List<SemanticRulesConfig.Rulebase.Builder> rules = new ArrayList<>(); + rules.add(create("child1", "vw -> audi;\n\n@include(parent.sr)\n\nvehiclebrand:audi -> vehiclebrand:skoda;\n\n")); + rules.add(create("child2", "@include(parent)\n\nvehiclebrand:vw -> vehiclebrand:audi;\n\n[brand] :- @super, skoda;\n\n\n")); + rules.add(create("cjk", "?? -> ???;\n@default\n")); + rules.add(create("grandchild", "@include(child1.sr)\n@include(child2.sr)\n\ncausesphrase -> \"a produced phrase\";\n")); + rules.add(create("grandfather", "[vehicle] :- car, motorcycle, bus;\n\ncars -> car;\n")); + rules.add(create("grandmother", "vehiclebrand:bmw +> expensivetv;\n")); + rules.add(create("parent", "@include(grandfather.sr)\n\n[brand] [vehicle] -> vehiclebrand:[brand];\n\n@include(grandmother.sr)\n\n[brand] :- alfa, audi, bmw;\n")); + + builder.rulebase(rules); + return builder.build(); + } + + private static SemanticRulesConfig.Rulebase.Builder create(String name, String rules) { + var ruleBaseBuilder = new SemanticRulesConfig.Rulebase.Builder(); + ruleBaseBuilder.name(name); + ruleBaseBuilder.rules(rules); + return ruleBaseBuilder; + } + } diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg b/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg index b040a44ad90..2e3299f7723 100644 --- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg +++ b/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg @@ -1,4 +1,3 @@ -compatibility false rulebase[1] rulebase[0].name "cjk" rulebase[0].isdefault false diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/semantic-rules.cfg b/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/semantic-rules.cfg deleted file mode 100644 index bdb824b0431..00000000000 --- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/semantic-rules.cfg +++ /dev/null @@ -1,15 +0,0 @@ -rulebase[7] -rulebase[0].name "child1" -rulebase[0].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\nvw -> audi;\n\n@include(parent.sr)\n\nvehiclebrand:audi -> vehiclebrand:skoda;\n\n" -rulebase[1].name "child2" -rulebase[1].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\n@include(parent)\n\nvehiclebrand:vw -> vehiclebrand:audi;\n\n[brand] :- @super, skoda;\n\n\n" -rulebase[2].name "cjk" -rulebase[2].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\n?? -> ???;\n@default\n" -rulebase[3].name "grandchild" -rulebase[3].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\n@include(child1.sr)\n@include(child2.sr)\n\ncausesphrase -> "a produced phrase";\n" -rulebase[4].name "grandfather" -rulebase[4].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\n[vehicle] :- car, motorcycle, bus;\n\ncars -> car;\n" -rulebase[5].name "grandmother" -rulebase[5].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\nvehiclebrand:bmw +> expensivetv;\n" -rulebase[6].name "parent" -rulebase[6].rules "# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.\n@include(grandfather.sr)\n\n[brand] [vehicle] -> vehiclebrand:[brand];\n\n@include(grandmother.sr)\n\n[brand] :- alfa, audi, bmw;\n" diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/JobType.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/JobType.java index b6905a97b5f..ab6a5a7eb23 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/JobType.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/JobType.java @@ -146,7 +146,7 @@ public final class JobType implements Comparable<JobType> { case "dev": return dev(parts[1]); case "perf": return perf(parts[1]); } - throw new IllegalArgumentException("job names must be 'system-test', 'staging-test', or <test|environment>-<region>, but got: " + jobName); + throw new IllegalArgumentException("job name must be 'system-test', 'staging-test', or <production|test|dev|perf>-<region>, but got: " + jobName); } public static List<JobType> allIn(ZoneRegistry zones) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java index 6429a8c3cca..f3581d56944 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java @@ -776,7 +776,7 @@ public class JobController { ApplicationPackage previous; try { previous = new ApplicationPackage(controller.applications().applicationStore().get(deploymentId, prevVersion)); - } catch (IllegalArgumentException e) { + } catch (RuntimeException e) { return ApplicationPackageDiff.diffAgainstEmpty(applicationPackage); } return ApplicationPackageDiff.diff(previous, applicationPackage); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index bead0663316..cc5438e9ed0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -986,12 +986,8 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler { private HttpResponse devApplicationPackage(ApplicationId id, JobType type) { ZoneId zone = type.zone(); RevisionId revision = controller.jobController().last(id, type).get().versions().targetRevision(); - try (InputStream applicationPackage = controller.applications().applicationStore().stream(new DeploymentId(id, zone), revision)) { - return new ZipResponse(id.toFullString() + "." + zone.value() + ".zip", applicationPackage); - } - catch (IOException e) { - throw new UncheckedIOException(e); - } + return new ZipResponse(id.toFullString() + "." + zone.value() + ".zip", + controller.applications().applicationStore().stream(new DeploymentId(id, zone), revision)); } private HttpResponse devApplicationPackageDiff(RunId runId) { @@ -1028,12 +1024,7 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler { InputStream applicationPackage = tests ? controller.applications().applicationStore().streamTester(tenantAndApplication.tenant(), tenantAndApplication.application(), revision) : controller.applications().applicationStore().stream(new DeploymentId(tenantAndApplication.defaultInstance(), ZoneId.defaultId()), revision); - try (applicationPackage) { - return new ZipResponse(filename, applicationPackage); - } - catch (IOException e) { - throw new UncheckedIOException(e); - } + return new ZipResponse(filename, applicationPackage); } private HttpResponse applicationPackageDiff(String tenant, String application, String number) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ZipResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ZipResponse.java index 5262d36ef11..f45ef49402b 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ZipResponse.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ZipResponse.java @@ -29,7 +29,9 @@ public class ZipResponse extends HttpResponse { @Override public void render(OutputStream outputStream) throws IOException { - zipContent.transferTo(outputStream); + try (zipContent) { + zipContent.transferTo(outputStream); + } } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java index 2726c778218..770957b19d2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java @@ -292,16 +292,9 @@ public class RoutingPolicies { // If all targets are configured OUT, all targets are kept IN. We do this because otherwise removing 100% of // the ALIAS records would cause the application endpoint to stop resolving entirely (NXDOMAIN). - for (var kv : targetsByEndpoint.entrySet()) { - Endpoint endpoint = kv.getKey(); - Set<Target> activeTargets = kv.getValue(); - if (!activeTargets.isEmpty()) { - continue; - } - Set<Target> inactiveTargets = inactiveTargetsByEndpoint.get(endpoint); - activeTargets.addAll(inactiveTargets); - inactiveTargets.clear(); - } + targetsByEndpoint.forEach((endpoint, targets) -> { + if (targets.isEmpty()) targets.addAll(inactiveTargetsByEndpoint.remove(endpoint)); + }); targetsByEndpoint.forEach((applicationEndpoint, targets) -> { // Where multiple zones are permitted, they all have the same routing policy, and nameServiceForwarder (below). 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 63313c6ed60..e9d8adf8ffb 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -372,7 +372,7 @@ public class Flags { public static final UnboundBooleanFlag NODE_ADMIN_TENANT_SERVICE_REGISTRY = defineFeatureFlag( "node-admin-tenant-service-registry", false, - List.of("olaa"), "2023-04-12", "2023-06-12", + List.of("olaa"), "2023-04-12", "2023-08-01", "Whether AthenzCredentialsMaintainer in node-admin should create tenant service identity certificate", "Takes effect on next tick", ZONE_ID, HOSTNAME, VESPA_VERSION, APPLICATION_ID diff --git a/fnet/src/examples/frt/rpc/rpc_callback_server.cpp b/fnet/src/examples/frt/rpc/rpc_callback_server.cpp index 5c21f73da7f..c0504f49c2b 100644 --- a/fnet/src/examples/frt/rpc/rpc_callback_server.cpp +++ b/fnet/src/examples/frt/rpc/rpc_callback_server.cpp @@ -7,10 +7,85 @@ #include <vespa/vespalib/util/signalhandler.h> #include <thread> +#include <mutex> +#include <condition_variable> +#include <future> #include <vespa/log/log.h> LOG_SETUP("rpc_callback_server"); +/** + * Class keeping track of 'detached' threads in order to wait for + * their completion on program shutdown. Threads are not actually + * detached, but perform co-operative auto-joining on completion. + **/ +class AutoJoiner +{ +private: + std::mutex _lock; + std::condition_variable _cond; + bool _closed; + size_t _pending; + std::thread _thread; + struct JoinGuard { + std::thread thread; + ~JoinGuard() { + if (thread.joinable()) { + assert(std::this_thread::get_id() != thread.get_id()); + thread.join(); + } + } + }; + void notify_start() { + std::lock_guard guard(_lock); + if (!_closed) { + ++_pending; + } else { + throw std::runtime_error("no new threads allowed"); + } + } + void notify_done(std::thread thread) { + JoinGuard join; + std::unique_lock guard(_lock); + join.thread = std::move(_thread); + _thread = std::move(thread); + if (--_pending == 0 && _closed) { + _cond.notify_all(); + } + } + auto wrap_task(auto task, std::promise<std::thread> &promise) { + return [future = promise.get_future(), task = std::move(task), &owner = *this]() mutable + { + auto thread = future.get(); + assert(std::this_thread::get_id() == thread.get_id()); + task(); + owner.notify_done(std::move(thread)); + }; + } +public: + AutoJoiner() : _lock(), _cond(), _closed(false), _pending(0), _thread() {} + ~AutoJoiner() { close_and_wait(); } + void start(auto task) { + notify_start(); + std::promise<std::thread> promise; + promise.set_value(std::thread(wrap_task(std::move(task), promise))); + }; + void close_and_wait() { + JoinGuard join; + std::unique_lock guard(_lock); + _closed = true; + while (_pending > 0) { + _cond.wait(guard); + } + std::swap(join.thread, _thread); + } +}; + +AutoJoiner &auto_joiner() { + static AutoJoiner obj; + return obj; +} + struct RPC : public FRT_Invokable { void CallBack(FRT_RPCRequest *req); @@ -35,7 +110,7 @@ void RPC::CallBack(FRT_RPCRequest *req) { req->Detach(); - std::thread(do_callback, req).detach(); + auto_joiner().start([req]{ do_callback(req); }); } void @@ -53,6 +128,7 @@ class MyApp { public: int main(int argc, char **argv); + ~MyApp() { auto_joiner().close_and_wait(); } }; int diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ArchiveResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ArchiveResponse.java index 6370b01af23..84c82d314c9 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ArchiveResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ArchiveResponse.java @@ -27,7 +27,7 @@ public class ArchiveResponse extends SlimeJsonResponse { archiveObject.setString("uri", entry.getValue()); }); archiveUris.accountArchiveUris().entrySet().stream() - .sorted() + .sorted(Map.Entry.comparingByKey()) .forEach(entry -> { Cursor archiveObject = archivesArray.addObject(); archiveObject.setString("account", entry.getKey().value()); diff --git a/searchlib/src/vespa/searchcommon/attribute/attributecontent.h b/searchlib/src/vespa/searchcommon/attribute/attributecontent.h index 696112450d5..0edc58bfcd5 100644 --- a/searchlib/src/vespa/searchcommon/attribute/attributecontent.h +++ b/searchlib/src/vespa/searchcommon/attribute/attributecontent.h @@ -9,6 +9,7 @@ namespace search::attribute { /** + * TODO Use SmallVector instead * This class is wrapping an array of type T and is used to hold the * attribute vector content for a given document. The values stored for the * given document in the attribute vector is copied into the array wrapped @@ -24,15 +25,13 @@ private: T * _dynamicBuf; uint32_t _size; uint32_t _capacity; - - AttributeContent(const AttributeContent & rhs); - AttributeContent & operator=(const AttributeContent & rhs); - public: + AttributeContent(const AttributeContent & rhs) = delete; + AttributeContent & operator=(const AttributeContent & rhs) = delete; /** * Creates a new object with an initial capacity of 16 without dynamic allocation. **/ - AttributeContent() : + AttributeContent() noexcept : _dynamicBuf(nullptr), _size(0), _capacity(16) @@ -52,7 +51,7 @@ public: * * @return iterator **/ - const T * begin() const { + const T * begin() const noexcept { if (_dynamicBuf != nullptr) { return _dynamicBuf; } @@ -64,7 +63,7 @@ public: * * @return iterator **/ - const T * end() const { + const T * end() const noexcept { return begin() + _size; } @@ -74,7 +73,7 @@ public: * @return read-only reference to the element * @param idx position into the underlying data **/ - const T & operator[](uint32_t idx) const { + const T & operator[](uint32_t idx) const noexcept { return *(begin() + idx); } @@ -83,25 +82,21 @@ public: * * @return number of elements used **/ - uint32_t size() const { - return _size; - } + uint32_t size() const noexcept { return _size; } /** * Returns the number of elements allocated in the underlying data array. * * @return number of elements allocated **/ - uint32_t capacity() const { - return _capacity; - } + uint32_t capacity() const noexcept { return _capacity; } /** * Returns a read/write pointer to the underlying data array. * * @return read/write pointer. **/ - T * data() { + T * data() noexcept { if (_dynamicBuf != nullptr) { return _dynamicBuf; } @@ -113,7 +108,7 @@ public: * * @param n number of elements used **/ - void setSize(uint32_t n) { + void setSize(uint32_t n) noexcept { _size = n; } @@ -141,8 +136,7 @@ public: * @param attribute the attribute vector * @param docId the docId **/ - void fill(const IAttributeVector & attribute, IAttributeVector::DocId docId) - { + void fill(const IAttributeVector & attribute, IAttributeVector::DocId docId) { uint32_t count = attribute.get(docId, data(), capacity()); while (count > capacity()) { allocate(count); diff --git a/searchlib/src/vespa/searchlib/aggregation/grouping.cpp b/searchlib/src/vespa/searchlib/aggregation/grouping.cpp index daff16e9029..b61cb585899 100644 --- a/searchlib/src/vespa/searchlib/aggregation/grouping.cpp +++ b/searchlib/src/vespa/searchlib/aggregation/grouping.cpp @@ -220,7 +220,7 @@ void Grouping::aggregate(const RankedHit * rankedHit, unsigned int len) preAggregate(isOrdered); HitsAggregationResult::SetOrdered pred; select(pred, pred); - if (_clock == NULL) { + if (_clock == nullptr) { aggregateWithoutClock(rankedHit, getMaxN(len)); } else { aggregateWithClock(rankedHit, getMaxN(len)); @@ -231,14 +231,14 @@ void Grouping::aggregate(const RankedHit * rankedHit, unsigned int len) void Grouping::aggregate(const RankedHit * rankedHit, unsigned int len, const BitVector * bVec) { preAggregate(false); - if (_clock == NULL) { + if (_clock == nullptr) { aggregateWithoutClock(rankedHit, getMaxN(len)); } else { aggregateWithClock(rankedHit, getMaxN(len)); } - if (bVec != NULL) { + if (bVec != nullptr) { unsigned int sz(bVec->size()); - if (_clock == NULL) { + if (_clock == nullptr) { if (getTopN() > 0) { for(DocId d(bVec->getFirstTrueBit()), i(0), m(getMaxN(sz)); (d < sz) && (i < m); d = bVec->getNextTrueBit(d+1), i++) { aggregate(d, 0.0); @@ -291,12 +291,12 @@ void Grouping::sortById() void Grouping::configureStaticStuff(const ConfigureStaticParams & params) { - if (params._attrCtx != NULL) { + if (params._attrCtx != nullptr) { AttributeNode::Configure confAttr(*params._attrCtx); select(confAttr, confAttr); } - if (params._docType != NULL) { + if (params._docType != nullptr) { DocumentAccessorNode::Configure confDoc(*params._docType); select(confDoc, confDoc); } diff --git a/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.cpp b/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.cpp index d94e8c19981..ac07a0d0140 100644 --- a/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.cpp +++ b/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.cpp @@ -1,157 +1,100 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "arrayatlookupfunctionnode.h" -#include "floatresultnode.h" -#include "integerresultnode.h" -#include "stringresultnode.h" -#include <vespa/searchcommon/attribute/iattributecontext.h> -#include <vespa/vespalib/util/stringfmt.h> namespace search::expression { using vespalib::Serializer; using vespalib::Deserializer; -IMPLEMENT_EXPRESSIONNODE(ArrayAtLookup, UnaryFunctionNode); +IMPLEMENT_EXPRESSIONNODE(ArrayAtLookup, AttributeNode); ArrayAtLookup::ArrayAtLookup() noexcept - : _attributeName(), - _attribute(nullptr), - _docId(0), - _basicAttributeType(BAT_STRING) + : AttributeNode(), + _currentIndex(), + _indexExpression() { + setCurrentIndex(&_currentIndex); } ArrayAtLookup::~ArrayAtLookup() = default; +ArrayAtLookup & ArrayAtLookup::operator=(const ArrayAtLookup &rhs) = default; -ArrayAtLookup::ArrayAtLookup(const vespalib::string &attribute, ExpressionNode::UP arg) - : UnaryFunctionNode(std::move(arg)), - _attributeName(attribute) +ArrayAtLookup::ArrayAtLookup(const vespalib::string &attribute, ExpressionNode::UP indexExpr) + : AttributeNode(attribute), + _currentIndex(), + _indexExpression(std::move(indexExpr)) { + setCurrentIndex(&_currentIndex); } ArrayAtLookup::ArrayAtLookup(const search::attribute::IAttributeVector &attr, - ExpressionNode::UP indexArg) - : UnaryFunctionNode(std::move(indexArg)), - _attributeName(attr.getName()), - _attribute(&attr) + ExpressionNode::UP indexExpr) + : AttributeNode(attr), + _currentIndex(), + _indexExpression(std::move(indexExpr)) { + setCurrentIndex(&_currentIndex); } - -ArrayAtLookup::ArrayAtLookup(const ArrayAtLookup &rhs) : - UnaryFunctionNode(rhs), - _attributeName(rhs._attributeName), - _attribute(rhs._attribute), - _docId(0), - _basicAttributeType(rhs._basicAttributeType) +ArrayAtLookup::ArrayAtLookup(const ArrayAtLookup &rhs) + : AttributeNode(rhs), + _currentIndex(), + _indexExpression(rhs._indexExpression) { + setCurrentIndex(&_currentIndex); } -ArrayAtLookup & ArrayAtLookup::operator= (const ArrayAtLookup &rhs) +bool +ArrayAtLookup::onExecute() const { - if (this != &rhs) { - UnaryFunctionNode::operator =(rhs); - _attributeName = rhs._attributeName; - _attribute = rhs._attribute; - _docId = 0; - _basicAttributeType = rhs._basicAttributeType; - } - return *this; + _indexExpression->execute(); + int64_t idx = _indexExpression->getResult()->getInteger(); + _currentIndex.set(idx); + AttributeNode::onExecute(); + return true; } -void ArrayAtLookup::onPrepareResult() +Serializer & +ArrayAtLookup::onSerialize(Serializer & os) const { - if (_attribute->isIntegerType()) { - _basicAttributeType = BAT_INT; - setResultType(std::make_unique<Int64ResultNode>()); - } else if (_attribute->isFloatingPointType()) { - _basicAttributeType = BAT_FLOAT; - setResultType(std::make_unique<FloatResultNode>()); - } else { - _basicAttributeType = BAT_STRING; - setResultType(std::make_unique<StringResultNode>()); - } + // Here we are doing a dirty skipping AttributeNode in the inheritance. + // This is due to refactoring and the need to keep serialization the same. + FunctionNode::onSerialize(os); + os << uint32_t(1u) << _indexExpression; // Simulating a single element vector. + os << _attributeName; + return os; } -bool ArrayAtLookup::onExecute() const +Deserializer & +ArrayAtLookup::onDeserialize(Deserializer & is) { - getArg().execute(); - int64_t idx = getArg().getResult()->getInteger(); - // get attribute data - size_t numValues = _attribute->getValueCount(_docId); - if (idx < 0) { - idx = 0; - } - if (idx >= (int64_t)numValues) { - idx = numValues - 1; - } - - if (_basicAttributeType == BAT_FLOAT) { - std::vector<search::attribute::IAttributeVector::WeightedFloat> wVector; - wVector.resize(numValues); - _attribute->get(_docId, &wVector[0], numValues); - std::vector<double> tmp; - tmp.resize(numValues); - for (size_t i = 0; i < numValues; ++i) { - tmp[i] = wVector[i].getValue(); - } - double result = 0; - if (idx >= 0 && idx < (int64_t)numValues) { - result = tmp[idx]; - } - static_cast<FloatResultNode &>(updateResult()).set(result); - } else if (_basicAttributeType == BAT_INT) { - std::vector<search::attribute::IAttributeVector::WeightedInt> wVector; - wVector.resize(numValues); - _attribute->get(_docId, &wVector[0], numValues); - std::vector<int64_t> tmp; - tmp.resize(numValues); - for (size_t i = 0; i < numValues; ++i) { - tmp[i] = wVector[i].getValue(); - } - int64_t result = 0; - if (idx >= 0 && idx < (int64_t)numValues) { - result = tmp[idx]; - } - static_cast<Int64ResultNode &>(updateResult()).set(result); + // See comment in onSerialize method. + FunctionNode::onDeserialize(is); + uint32_t count(0); + is >> count; + if (count > 0) { + is >> _indexExpression; } else { - std::vector<search::attribute::IAttributeVector::WeightedString> wVector; - wVector.resize(numValues); - _attribute->get(_docId, &wVector[0], numValues); - std::vector<vespalib::string> tmp; - tmp.resize(numValues); - for (size_t i = 0; i < numValues; ++i) { - tmp[i] = wVector[i].getValue(); - } - vespalib::string result; - if (idx >= 0 && idx < (int64_t)numValues) { - result = tmp[idx]; - } - static_cast<StringResultNode &>(updateResult()).set(result); - } - return true; -} - -void ArrayAtLookup::wireAttributes(const search::attribute::IAttributeContext & attrCtx) -{ - _attribute = attrCtx.getAttribute(_attributeName); - if (_attribute == nullptr) { - throw std::runtime_error(vespalib::make_string("Failed locating attribute vector '%s'", _attributeName.c_str())); + _indexExpression.reset(); } + is >> _attributeName; + return is; } -Serializer & ArrayAtLookup::onSerialize(Serializer & os) const +void +ArrayAtLookup::visitMembers(vespalib::ObjectVisitor &visitor) const { - UnaryFunctionNode::onSerialize(os); - os << _attributeName; - return os; + AttributeNode::visitMembers(visitor); + visit(visitor, "index", *_indexExpression); } -Deserializer & ArrayAtLookup::onDeserialize(Deserializer & is) +void +ArrayAtLookup::selectMembers(const vespalib::ObjectPredicate & predicate, vespalib::ObjectOperation & operation) { - UnaryFunctionNode::onDeserialize(is); - is >> _attributeName; - return is; + AttributeNode::selectMembers(predicate, operation); + if (_indexExpression) { + _indexExpression->selectMembers(predicate, operation); + } } } diff --git a/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.h b/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.h index 9404ec09b04..c884c6bf1af 100644 --- a/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.h +++ b/searchlib/src/vespa/searchlib/expression/arrayatlookupfunctionnode.h @@ -1,7 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once -#include "unaryfunctionnode.h" +#include "attributenode.h" namespace search::attribute { class IAttributeVector; @@ -10,7 +10,7 @@ namespace search::attribute { namespace search::expression { -class ArrayAtLookup : public UnaryFunctionNode +class ArrayAtLookup : public AttributeNode { public: DECLARE_EXPRESSIONNODE(ArrayAtLookup); @@ -22,20 +22,12 @@ public: ArrayAtLookup(const search::attribute::IAttributeVector &attr, ExpressionNode::UP indexArg); ArrayAtLookup(const ArrayAtLookup &rhs); ArrayAtLookup & operator= (const ArrayAtLookup &rhs); - void setDocId(DocId docId) { _docId = docId; } -private: bool onExecute() const override; - void onPrepareResult() override; - void wireAttributes(const search::attribute::IAttributeContext &attrCtx) override; - - enum BasicAttributeType { - BAT_INT, BAT_FLOAT, BAT_STRING - }; - - vespalib::string _attributeName; - const search::attribute::IAttributeVector * _attribute; - DocId _docId; - BasicAttributeType _basicAttributeType; + void visitMembers(vespalib::ObjectVisitor & visitor) const override; + void selectMembers(const vespalib::ObjectPredicate & predicate, vespalib::ObjectOperation & operation) override; +private: + mutable CurrentIndex _currentIndex; + ExpressionNode::CP _indexExpression; }; } diff --git a/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp b/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp index c1f61afdfd5..5302aaaf89a 100644 --- a/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp +++ b/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp @@ -25,10 +25,7 @@ class AttributeMapLookupNode::KeyHandler protected: const IAttributeVector &_attribute; - KeyHandler(const IAttributeVector &attribute) - : _attribute(attribute) - { - } + KeyHandler(const IAttributeVector &attribute) noexcept : _attribute(attribute) { } public: static uint32_t noKeyIdx() { return std::numeric_limits<uint32_t>::max(); } virtual ~KeyHandler() = default; @@ -40,15 +37,13 @@ namespace { class BadKeyHandler : public AttributeMapLookupNode::KeyHandler { public: - BadKeyHandler(const IAttributeVector &attribute) - : KeyHandler(attribute) - { - } + BadKeyHandler(const IAttributeVector &attribute) noexcept : KeyHandler(attribute) { } uint32_t handle(DocId) override { return noKeyIdx(); } }; template <typename KeyType> -KeyType convertKey(const IAttributeVector &, const vespalib::string &key) +KeyType +convertKey(const IAttributeVector &, const vespalib::string &key) { KeyType ret; vespalib::asciistream is(key); @@ -57,13 +52,15 @@ KeyType convertKey(const IAttributeVector &, const vespalib::string &key) } template <> -vespalib::string convertKey<vespalib::string>(const IAttributeVector &, const vespalib::string &key) +vespalib::string +convertKey<vespalib::string>(const IAttributeVector &, const vespalib::string &key) { return key; } template <> -EnumHandle convertKey<EnumHandle>(const IAttributeVector &attribute, const vespalib::string &key) +EnumHandle +convertKey<EnumHandle>(const IAttributeVector &attribute, const vespalib::string &key) { EnumHandle ret; if (!attribute.findEnum(key.c_str(), ret)) { @@ -83,8 +80,7 @@ public: : KeyHandler(attribute), _keys(), _key(convertKey<KeyType>(attribute, key)) - { - } + { } ~KeyHandlerT() override; uint32_t handle(DocId docId) override { _keys.fill(_attribute, docId); @@ -107,15 +103,13 @@ using EnumKeyHandler = KeyHandlerT<EnumHandle>; template <typename T> bool -matchingKey(T lhs, T rhs) -{ +matchingKey(T lhs, T rhs) { return lhs == rhs; } template <> bool -matchingKey<const char *>(const char *lhs, const char *rhs) -{ +matchingKey<const char *>(const char *lhs, const char *rhs) { return (strcmp(lhs, rhs) == 0); } @@ -130,8 +124,7 @@ public: : KeyHandler(attribute), _keySourceAttribute(keySourceAttribute), _keys() - { - } + { } ~IndirectKeyHandlerT() override; uint32_t handle(DocId docId) override { T key = T(); @@ -158,11 +151,10 @@ class ValueHandler : public AttributeNode::Handler protected: std::unique_ptr<AttributeMapLookupNode::KeyHandler> _keyHandler; const IAttributeVector &_attribute; - ValueHandler(std::unique_ptr<AttributeMapLookupNode::KeyHandler> keyHandler, const IAttributeVector &attribute) + ValueHandler(std::unique_ptr<AttributeMapLookupNode::KeyHandler> keyHandler, const IAttributeVector &attribute) noexcept : _keyHandler(std::move(keyHandler)), _attribute(attribute) - { - } + { } }; template <typename T, typename ResultNodeType> @@ -172,13 +164,12 @@ class ValueHandlerT : public ValueHandler ResultNodeType &_result; T _undefinedValue; public: - ValueHandlerT(std::unique_ptr<AttributeMapLookupNode::KeyHandler> keyHandler, const IAttributeVector &attribute, ResultNodeType &result, T undefinedValue) + ValueHandlerT(std::unique_ptr<AttributeMapLookupNode::KeyHandler> keyHandler, const IAttributeVector &attribute, ResultNodeType &result, T undefinedValue) noexcept : ValueHandler(std::move(keyHandler), attribute), _values(), _result(result), _undefinedValue(undefinedValue) - { - } + { } void handle(const AttributeResult & r) override { uint32_t docId = r.getDocId(); uint32_t keyIdx = _keyHandler->handle(docId); @@ -199,7 +190,8 @@ using FloatValueHandler = ValueHandlerT<double, FloatResultNode>; using StringValueHandler = ValueHandlerT<const char *, StringResultNode>; using EnumValueHandler = ValueHandlerT<EnumHandle, EnumResultNode>; -const IAttributeVector *findAttribute(const search::attribute::IAttributeContext &attrCtx, bool useEnumOptimization, const vespalib::string &name) +const IAttributeVector * +findAttribute(const search::attribute::IAttributeContext &attrCtx, bool useEnumOptimization, const vespalib::string &name) { const IAttributeVector *attribute = useEnumOptimization ? attrCtx.getAttributeStableEnum(name) : attrCtx.getAttribute(name); if (attribute == nullptr) { @@ -208,7 +200,8 @@ const IAttributeVector *findAttribute(const search::attribute::IAttributeContext return attribute; } -IAttributeVector::largeint_t getUndefinedValue(BasicType::Type basicType) +IAttributeVector::largeint_t +getUndefinedValue(BasicType::Type basicType) { switch (basicType) { case BasicType::INT8: @@ -234,8 +227,7 @@ AttributeMapLookupNode::AttributeMapLookupNode() _keySourceAttributeName(), _keyAttribute(nullptr), _keySourceAttribute(nullptr) -{ -} +{ } AttributeMapLookupNode::AttributeMapLookupNode(const AttributeMapLookupNode &) = default; @@ -247,8 +239,7 @@ AttributeMapLookupNode::AttributeMapLookupNode(vespalib::stringref name, vespali _keySourceAttributeName(keySourceAttributeName), _keyAttribute(nullptr), _keySourceAttribute(nullptr) -{ -} +{ } AttributeMapLookupNode::~AttributeMapLookupNode() = default; @@ -377,13 +368,15 @@ AttributeMapLookupNode::wireAttributes(const search::attribute::IAttributeContex } } -Serializer & AttributeMapLookupNode::onSerialize(Serializer & os) const +Serializer & +AttributeMapLookupNode::onSerialize(Serializer & os) const { AttributeNode::onSerialize(os); return os << _keyAttributeName << _valueAttributeName << _key << _keySourceAttributeName; } -Deserializer & AttributeMapLookupNode::onDeserialize(Deserializer & is) +Deserializer & +AttributeMapLookupNode::onDeserialize(Deserializer & is) { AttributeNode::onDeserialize(is); return is >> _keyAttributeName >> _valueAttributeName >> _key >> _keySourceAttributeName; diff --git a/searchlib/src/vespa/searchlib/expression/attributenode.cpp b/searchlib/src/vespa/searchlib/expression/attributenode.cpp index 7e46de934f0..5fc17380ee4 100644 --- a/searchlib/src/vespa/searchlib/expression/attributenode.cpp +++ b/searchlib/src/vespa/searchlib/expression/attributenode.cpp @@ -4,6 +4,7 @@ #include "resultvector.h" #include "enumattributeresult.h" #include <vespa/searchcommon/attribute/iattributecontext.h> +#include <cassert> namespace search::expression { @@ -18,10 +19,10 @@ template <typename V> class AttributeNode::IntegerHandler : public AttributeNode::Handler { public: - IntegerHandler(ResultNode & result) noexcept : - Handler(), - _vector(((V &)result).getVector()), - _wVector() + IntegerHandler(ResultNode & result) noexcept + : Handler(), + _vector(((V &)result).getVector()), + _wVector() { } void handle(const AttributeResult & r) override; private: @@ -32,10 +33,10 @@ private: class AttributeNode::FloatHandler : public AttributeNode::Handler { public: - FloatHandler(ResultNode & result) noexcept : - Handler(), - _vector(((FloatResultNodeVector &)result).getVector()), - _wVector() + FloatHandler(ResultNode & result) noexcept + : Handler(), + _vector(((FloatResultNodeVector &)result).getVector()), + _wVector() { } void handle(const AttributeResult & r) override; private: @@ -46,10 +47,10 @@ private: class AttributeNode::StringHandler : public AttributeNode::Handler { public: - StringHandler(ResultNode & result) noexcept : - Handler(), - _vector(((StringResultNodeVector &)result).getVector()), - _wVector() + StringHandler(ResultNode & result) noexcept + : Handler(), + _vector(((StringResultNodeVector &)result).getVector()), + _wVector() { } void handle(const AttributeResult & r) override; private: @@ -60,10 +61,10 @@ private: class AttributeNode::EnumHandler : public AttributeNode::Handler { public: - EnumHandler(ResultNode & result) noexcept : - Handler(), - _vector(((EnumResultNodeVector &)result).getVector()), - _wVector() + EnumHandler(ResultNode & result) noexcept + : Handler(), + _vector(((EnumResultNodeVector &)result).getVector()), + _wVector() { } void handle(const AttributeResult & r) override; private: @@ -85,43 +86,70 @@ createResult(const IAttributeVector * attribute) return std::make_unique<EnumAttributeResult>(enumRefs, attribute, 0); } +template<typename T> +std::pair<std::unique_ptr<ResultNode>, std::unique_ptr<AttributeNode::Handler>> +createSingle() { + return { std::make_unique<T>(), std::unique_ptr<AttributeNode::Handler>()}; +} + +template<typename T, typename H> +std::pair<std::unique_ptr<ResultNode>, std::unique_ptr<AttributeNode::Handler>> +createMulti() { + auto result = std::make_unique<T>(); + auto handler = std::make_unique<H>(*result); + return { std::move(result), std::move(handler)}; +} + } AttributeNode::AttributeNode() : FunctionNode(), _scratchResult(std::make_unique<AttributeResult>()), + _index(nullptr), + _keepAliveForIndexLookups(), _hasMultiValue(false), _useEnumOptimization(false), + _needExecute(true), _handler(), _attributeName() {} AttributeNode::~AttributeNode() = default; -AttributeNode::AttributeNode(vespalib::stringref name) : - FunctionNode(), - _scratchResult(std::make_unique<AttributeResult>()), - _hasMultiValue(false), - _useEnumOptimization(false), - _handler(), - _attributeName(name) +AttributeNode::AttributeNode(vespalib::stringref name) + : FunctionNode(), + _scratchResult(std::make_unique<AttributeResult>()), + _index(nullptr), + _keepAliveForIndexLookups(), + _hasMultiValue(false), + _useEnumOptimization(false), + _needExecute(true), + _handler(), + _attributeName(name) {} -AttributeNode::AttributeNode(const IAttributeVector & attribute) : - FunctionNode(), - _scratchResult(createResult(&attribute)), - _hasMultiValue(attribute.hasMultiValue()), - _useEnumOptimization(false), - _handler(), - _attributeName(attribute.getName()) + +AttributeNode::AttributeNode(const IAttributeVector & attribute) + : FunctionNode(), + _scratchResult(createResult(&attribute)), + _index(nullptr), + _keepAliveForIndexLookups(), + _hasMultiValue(attribute.hasMultiValue()), + _useEnumOptimization(false), + _needExecute(true), + _handler(), + _attributeName(attribute.getName()) {} -AttributeNode::AttributeNode(const AttributeNode & attribute) : - FunctionNode(attribute), - _scratchResult(attribute._scratchResult->clone()), - _hasMultiValue(attribute._hasMultiValue), - _useEnumOptimization(attribute._useEnumOptimization), - _handler(), - _attributeName(attribute._attributeName) +AttributeNode::AttributeNode(const AttributeNode & attribute) + : FunctionNode(attribute), + _scratchResult(attribute._scratchResult->clone()), + _index(nullptr), + _keepAliveForIndexLookups(), + _hasMultiValue(attribute._hasMultiValue), + _useEnumOptimization(attribute._useEnumOptimization), + _needExecute(true), + _handler(), + _attributeName(attribute._attributeName) { _scratchResult->setDocId(0); } @@ -136,105 +164,98 @@ AttributeNode::operator = (const AttributeNode & attr) _useEnumOptimization = attr._useEnumOptimization; _scratchResult.reset(attr._scratchResult->clone()); _scratchResult->setDocId(0); + _handler.reset(); + _keepAliveForIndexLookups.reset(); + _needExecute = true; } return *this; } -void -AttributeNode::onPrepare(bool preserveAccurateTypes) -{ - const IAttributeVector * attribute = _scratchResult->getAttribute(); - if (attribute != nullptr) { - BasicType::Type basicType = attribute->getBasicType(); - if (attribute->isIntegerType()) { - if (_hasMultiValue) { - if (basicType == BasicType::BOOL) { - setResultType(std::make_unique<BoolResultNodeVector>()); - _handler = std::make_unique<IntegerHandler<BoolResultNodeVector>>(updateResult()); - } else if (preserveAccurateTypes) { - switch (basicType) { - case BasicType::INT8: - setResultType(std::make_unique<Int8ResultNodeVector>()); - _handler = std::make_unique<IntegerHandler<Int8ResultNodeVector>>(updateResult()); - break; - case BasicType::INT16: - setResultType(std::make_unique<Int16ResultNodeVector>()); - _handler = std::make_unique<IntegerHandler<Int16ResultNodeVector>>(updateResult()); - break; - case BasicType::INT32: - setResultType(std::make_unique<Int32ResultNodeVector>()); - _handler = std::make_unique<IntegerHandler<Int32ResultNodeVector>>(updateResult()); - break; - case BasicType::INT64: - setResultType(std::make_unique<Int64ResultNodeVector>()); - _handler = std::make_unique<IntegerHandler<Int64ResultNodeVector>>(updateResult()); - break; - default: - throw std::runtime_error("This is no valid integer attribute " + attribute->getName()); - break; - } - } else { - setResultType(std::make_unique<IntegerResultNodeVector>()); - _handler = std::make_unique<IntegerHandler<IntegerResultNodeVector>>(updateResult()); +std::pair<std::unique_ptr<ResultNode>, std::unique_ptr<AttributeNode::Handler>> +AttributeNode::createResultAndHandler(bool preserveAccurateTypes, const attribute::IAttributeVector & attribute) const { + BasicType::Type basicType = attribute.getBasicType(); + if (attribute.isIntegerType()) { + if (_hasMultiValue) { + if (basicType == BasicType::BOOL) { + return createMulti<BoolResultNodeVector, IntegerHandler<BoolResultNodeVector>>(); + } else if (preserveAccurateTypes) { + switch (basicType) { + case BasicType::INT8: + return createMulti<Int8ResultNodeVector, IntegerHandler<Int8ResultNodeVector>>(); + case BasicType::INT16: + return createMulti<Int16ResultNodeVector, IntegerHandler<Int16ResultNodeVector>>(); + case BasicType::INT32: + return createMulti<Int32ResultNodeVector, IntegerHandler<Int32ResultNodeVector>>(); + case BasicType::INT64: + return createMulti<Int64ResultNodeVector, IntegerHandler<Int64ResultNodeVector>>(); + default: + throw std::runtime_error("This is no valid integer attribute " + attribute.getName()); } } else { - if (basicType == BasicType::BOOL) { - setResultType(std::make_unique<BoolResultNode>()); - } else if (preserveAccurateTypes) { - switch (basicType) { - case BasicType::INT8: - setResultType(std::make_unique<Int8ResultNode>()); - break; - case BasicType::INT16: - setResultType(std::make_unique<Int16ResultNode>()); - break; - case BasicType::INT32: - setResultType(std::make_unique<Int32ResultNode>()); - break; - case BasicType::INT64: - setResultType(std::make_unique<Int64ResultNode>()); - break; - default: - throw std::runtime_error("This is no valid integer attribute " + attribute->getName()); - break; - } - } else { - setResultType(std::make_unique<Int64ResultNode>()); - } + return createMulti<IntegerResultNodeVector, IntegerHandler<IntegerResultNodeVector>>(); } - } else if (attribute->isFloatingPointType()) { - if (_hasMultiValue) { - setResultType(std::make_unique<FloatResultNodeVector>()); - _handler = std::make_unique<FloatHandler>(updateResult()); - } else { - setResultType(std::make_unique<FloatResultNode>()); - } - } else if (attribute->isStringType()) { - if (_hasMultiValue) { - if (_useEnumOptimization) { - setResultType(std::make_unique<EnumResultNodeVector>()); - _handler = std::make_unique<EnumHandler>(updateResult()); - } else { - setResultType(std::make_unique<StringResultNodeVector>()); - _handler = std::make_unique<StringHandler>(updateResult()); - } - } else { - if (_useEnumOptimization) { - setResultType(std::make_unique<EnumResultNode>()); - } else { - setResultType(std::make_unique<StringResultNode>()); + } else { + if (basicType == BasicType::BOOL) { + return createSingle<BoolResultNode>(); + } else if (preserveAccurateTypes) { + switch (basicType) { + case BasicType::INT8: + return createSingle<Int8ResultNode>(); + case BasicType::INT16: + return createSingle<Int16ResultNode>(); + case BasicType::INT32: + return createSingle<Int32ResultNode>(); + case BasicType::INT64: + return createSingle<Int64ResultNode>(); + default: + throw std::runtime_error("This is no valid integer attribute " + attribute.getName()); } - } - } else if (attribute->is_raw_type()) { - if (_hasMultiValue) { - throw std::runtime_error(make_string("Does not support multivalue raw attribute vector '%s'", - attribute->getName().c_str())); } else { - setResultType(std::make_unique<RawResultNode>()); + return createSingle<Int64ResultNode>(); } + } + } else if (attribute.isFloatingPointType()) { + return (_hasMultiValue) + ? createMulti<FloatResultNodeVector, FloatHandler>() + : createSingle<FloatResultNode>(); + } else if (attribute.isStringType()) { + if (_hasMultiValue) { + return (_useEnumOptimization) + ? createMulti<EnumResultNodeVector, EnumHandler>() + : createMulti<StringResultNodeVector, StringHandler>(); + } else { + return (_useEnumOptimization) + ? createSingle<EnumResultNode>() + : createSingle<StringResultNode>(); + } + } else if (attribute.is_raw_type()) { + if (_hasMultiValue) { + throw std::runtime_error(make_string("Does not support multivalue raw attribute vector '%s'", + attribute.getName().c_str())); + } else { + return createSingle<RawResultNode>(); + } + } else { + throw std::runtime_error(make_string("Can not deduce correct resultclass for attribute vector '%s'", + attribute.getName().c_str())); + } +} + +void +AttributeNode::onPrepare(bool preserveAccurateTypes) +{ + const IAttributeVector * attribute = getAttribute(); + if (attribute != nullptr) { + auto[result, handler] = createResultAndHandler(preserveAccurateTypes, *attribute); + _handler = std::move(handler); + if (_index == nullptr) { + setResultType(std::move(result)); } else { - throw std::runtime_error(make_string("Can not deduce correct resultclass for attribute vector '%s'", - attribute->getName().c_str())); + assert(_hasMultiValue); + assert(_handler); + setResultType(result->createBaseType()); + assert(result->inherits(ResultNodeVector::classId)); + _keepAliveForIndexLookups.reset(dynamic_cast<ResultNodeVector *>(result.release())); } } } @@ -276,7 +297,8 @@ AttributeNode::StringHandler::handle(const AttributeResult & r) } } -void AttributeNode::EnumHandler::handle(const AttributeResult & r) +void +AttributeNode::EnumHandler::handle(const AttributeResult & r) { size_t numValues = r.getAttribute()->getValueCount(r.getDocId()); _vector.resize(numValues); @@ -287,10 +309,25 @@ void AttributeNode::EnumHandler::handle(const AttributeResult & r) } } -bool AttributeNode::onExecute() const +void +AttributeNode::setDocId(DocId docId) { + _scratchResult->setDocId(docId); + _needExecute = true; +} + +bool +AttributeNode::onExecute() const { if (_handler) { - _handler->handle(*_scratchResult); + if (_needExecute) { + _handler->handle(*_scratchResult); + _needExecute = false; + } + if ((_index != nullptr) && !_keepAliveForIndexLookups->empty()) { + assert(_hasMultiValue); + size_t idx = std::min(size_t(_index->get()), _keepAliveForIndexLookups->size() - 1); + updateResult().set(_keepAliveForIndexLookups->get(idx)); + } } else { updateResult().set(*_scratchResult); } diff --git a/searchlib/src/vespa/searchlib/expression/attributenode.h b/searchlib/src/vespa/searchlib/expression/attributenode.h index 67ec6a3302f..bb8e574d853 100644 --- a/searchlib/src/vespa/searchlib/expression/attributenode.h +++ b/searchlib/src/vespa/searchlib/expression/attributenode.h @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once +#include "currentindex.h" #include "functionnode.h" #include "attributeresult.h" #include <vespa/vespalib/objects/objectoperation.h> @@ -19,7 +20,7 @@ public: class Configure : public vespalib::ObjectOperation, public vespalib::ObjectPredicate { public: - Configure(const search::attribute::IAttributeContext & attrCtx) : _attrCtx(attrCtx) { } + Configure(const attribute::IAttributeContext & attrCtx) : _attrCtx(attrCtx) { } private: void execute(vespalib::Identifiable &obj) override { static_cast<ExpressionNode &>(obj).wireAttributes(_attrCtx); @@ -28,7 +29,7 @@ public: bool check(const vespalib::Identifiable &obj) const override { return obj.inherits(ExpressionNode::classId); } - const search::attribute::IAttributeContext & _attrCtx; + const attribute::IAttributeContext & _attrCtx; }; class CleanupAttributeReferences : public vespalib::ObjectOperation, public vespalib::ObjectPredicate @@ -42,12 +43,13 @@ public: DECLARE_EXPRESSIONNODE(AttributeNode); AttributeNode(); AttributeNode(vespalib::stringref name); - AttributeNode(const search::attribute::IAttributeVector & attribute); + AttributeNode(const attribute::IAttributeVector & attribute); AttributeNode(const AttributeNode & attribute); AttributeNode & operator = (const AttributeNode & attribute); ~AttributeNode() override; - void setDocId(DocId docId) const { _scratchResult->setDocId(docId); } - const search::attribute::IAttributeVector *getAttribute() const { + void setDocId(DocId docId); + void setCurrentIndex(const CurrentIndex * index) { _index = index; } + const attribute::IAttributeVector *getAttribute() const { return _scratchResult ? _scratchResult->getAttribute() : nullptr; } const vespalib::string & getAttributeName() const { return _attributeName; } @@ -62,21 +64,26 @@ public: virtual void handle(const AttributeResult & r) = 0; }; private: + std::pair<std::unique_ptr<ResultNode>, std::unique_ptr<Handler>> + createResultAndHandler(bool preserveAccurateType, const attribute::IAttributeVector & attribute) const; template <typename V> class IntegerHandler; class FloatHandler; class StringHandler; class EnumHandler; protected: virtual void cleanup(); - void wireAttributes(const search::attribute::IAttributeContext & attrCtx) override; + void wireAttributes(const attribute::IAttributeContext & attrCtx) override; void onPrepare(bool preserveAccurateTypes) override; bool onExecute() const override; - std::unique_ptr<AttributeResult> _scratchResult; - bool _hasMultiValue; - bool _useEnumOptimization; - std::unique_ptr<Handler> _handler; - vespalib::string _attributeName; + std::unique_ptr<AttributeResult> _scratchResult; + const CurrentIndex *_index; + std::unique_ptr<ResultNodeVector> _keepAliveForIndexLookups; + bool _hasMultiValue; + bool _useEnumOptimization; + mutable bool _needExecute; + std::unique_ptr<Handler> _handler; + vespalib::string _attributeName; }; } diff --git a/searchlib/src/vespa/searchlib/expression/currentindex.h b/searchlib/src/vespa/searchlib/expression/currentindex.h new file mode 100644 index 00000000000..e0ed820c916 --- /dev/null +++ b/searchlib/src/vespa/searchlib/expression/currentindex.h @@ -0,0 +1,21 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include <cstdint> +#include <limits> + +namespace search::expression { + +class CurrentIndex { +public: + CurrentIndex() noexcept : _index(0) {} + uint32_t get() const noexcept { return _index; } + void set(int64_t index) noexcept { + _index = (index > 0) ? index : 0; + } +private: + uint32_t _index; +}; + +} diff --git a/searchlib/src/vespa/searchlib/expression/functionnodes.cpp b/searchlib/src/vespa/searchlib/expression/functionnodes.cpp index 109b4a59a05..60574a355d0 100644 --- a/searchlib/src/vespa/searchlib/expression/functionnodes.cpp +++ b/searchlib/src/vespa/searchlib/expression/functionnodes.cpp @@ -149,21 +149,20 @@ ArithmeticTypeConversion::getType(const ResultNode & arg1, const ResultNode & ar { size_t baseTypeId = getType(getBaseType2(arg1), getBaseType2(arg2)); size_t dimension = std::max(getDimension(arg1), getDimension(arg2)); - ResultNode::UP result; if (dimension == 0) { return ResultNode::UP(static_cast<ResultNode *>(Identifiable::classFromId(baseTypeId)->create())); } else if (dimension == 1) { if (baseTypeId == Int64ResultNode::classId) { - result.reset(new IntegerResultNodeVector()); + return std::make_unique<IntegerResultNodeVector>(); } else if (baseTypeId == FloatResultNode::classId) { - result.reset(new FloatResultNodeVector()); + return std::make_unique<FloatResultNodeVector>(); } else { throw std::runtime_error("We can not handle anything but numbers."); } } else { throw std::runtime_error("We are not able to handle multidimensional arrays"); } - return result; + return ResultNode::UP(); } ResultNode::UP diff --git a/security-utils/pom.xml b/security-utils/pom.xml index a6f0040509c..4fde0cb7b56 100644 --- a/security-utils/pom.xml +++ b/security-utils/pom.xml @@ -81,6 +81,7 @@ <Bundle-SymbolicName>${project.artifactId}</Bundle-SymbolicName> <Bundle-Version>${parsedVersion.majorVersion}.${parsedVersion.minorVersion}.${parsedVersion.incrementalVersion}</Bundle-Version> <Export-Package>com.yahoo.security.*;version=1.0.0;-noimport:=true</Export-Package> + <X-JDisc-Non-PublicApi-Export-Package>com.yahoo.security,com.yahoo.security.tls</X-JDisc-Non-PublicApi-Export-Package> <_nouses>true</_nouses> <!-- Don't include 'uses' directives for package exports --> <_fixupmessages>"Classes found in the wrong directory"</_fixupmessages> <!-- Hide warnings for bouncycastle multi-release jars --> </instructions> diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h index b1bb7aa95d6..ba7f8dcb2a5 100644 --- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h +++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h @@ -39,11 +39,6 @@ public: BTreeLockableMap(); ~BTreeLockableMap() override; - bool operator==(const BTreeLockableMap& other) const; - bool operator!=(const BTreeLockableMap& other) const { - return ! (*this == other); - } - bool operator<(const BTreeLockableMap& other) const; size_t size() const noexcept override; size_t getMemoryUsage() const noexcept override; vespalib::MemoryUsage detailed_memory_usage() const noexcept override; diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp index 6a37b771c48..b92b3481845 100644 --- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp +++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp @@ -92,52 +92,6 @@ size_t BTreeLockableMap<T>::LockWaiters::insert(const LockId & lid) { } template <typename T> -bool BTreeLockableMap<T>::operator==(const BTreeLockableMap& other) const { - std::lock_guard guard(_lock); - std::lock_guard guard2(other._lock); - if (_impl->size() != other._impl->size()) { - return false; - } - auto lhs = _impl->begin(); - auto rhs = other._impl->begin(); - for (; lhs.valid(); ++lhs, ++rhs) { - assert(rhs.valid()); - if (lhs.getKey() != rhs.getKey()) { - return false; - } - if (_impl->const_value_ref_from_valid_iterator(lhs) - != other._impl->const_value_ref_from_valid_iterator(rhs)) - { - return false; - } - } - return true; -} - -template <typename T> -bool BTreeLockableMap<T>::operator<(const BTreeLockableMap& other) const { - std::lock_guard guard(_lock); - std::lock_guard guard2(other._lock); - auto lhs = _impl->begin(); - auto rhs = other._impl->begin(); - for (; lhs.valid() && rhs.valid(); ++lhs, ++rhs) { - if (lhs.getKey() != rhs.getKey()) { - return (lhs.getKey() < rhs.getKey()); - } - if (_impl->const_value_ref_from_valid_iterator(lhs) - != other._impl->const_value_ref_from_valid_iterator(rhs)) - { - return (_impl->const_value_ref_from_valid_iterator(lhs) - < other._impl->const_value_ref_from_valid_iterator(rhs)); - } - } - if (lhs.valid() == rhs.valid()) { - return false; // All keys are equal in maps of equal size. - } - return rhs.valid(); // Rhs still valid, lhs is not; ergo lhs is "less". -} - -template <typename T> size_t BTreeLockableMap<T>::size() const noexcept { std::lock_guard guard(_lock); return _impl->size(); diff --git a/storage/src/vespa/storage/bucketdb/storagebucketinfo.h b/storage/src/vespa/storage/bucketdb/storagebucketinfo.h index fe9d197f0e9..220e1cc037e 100644 --- a/storage/src/vespa/storage/bucketdb/storagebucketinfo.h +++ b/storage/src/vespa/storage/bucketdb/storagebucketinfo.h @@ -10,23 +10,12 @@ struct StorageBucketInfo { api::BucketInfo info; StorageBucketInfo() noexcept : info() {} - static bool mayContain(const StorageBucketInfo&) { return true; } void print(std::ostream&, bool verbose, const std::string& indent) const; - bool valid() const { return info.valid(); } - void setBucketInfo(const api::BucketInfo& i) { info = i; } - const api::BucketInfo& getBucketInfo() const { return info; } - void setEmptyWithMetaData() { - info.setChecksum(1); - info.setMetaCount(1); - info.setDocumentCount(0); - info.setTotalDocumentSize(0); - } - bool verifyLegal() const { return true; } - uint32_t getMetaCount() const { return info.getMetaCount(); } - void setChecksum(uint32_t crc) { info.setChecksum(crc); } - bool operator == (const StorageBucketInfo & b) const; - bool operator != (const StorageBucketInfo & b) const; - bool operator < (const StorageBucketInfo & b) const; + bool valid() const noexcept { return info.valid(); } + void setBucketInfo(const api::BucketInfo& i) noexcept { info = i; } + const api::BucketInfo& getBucketInfo() const noexcept { return info; } + bool verifyLegal() const noexcept { return true; } + uint32_t getMetaCount() const noexcept { return info.getMetaCount(); } }; std::ostream& operator<<(std::ostream& out, const StorageBucketInfo& info); diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp index d80c6734c36..e2b7b7e1e69 100644 --- a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp +++ b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp @@ -20,18 +20,6 @@ print(std::ostream& out, bool, const std::string&) const out << info; } -bool StorageBucketInfo::operator==(const StorageBucketInfo& ) const { - return true; -} - -bool StorageBucketInfo::operator!=(const StorageBucketInfo& b) const { - return !(*this == b); -} - -bool StorageBucketInfo::operator<(const StorageBucketInfo& ) const { - return false; -} - std::ostream& operator<<(std::ostream& out, const StorageBucketInfo& info) { info.print(out, false, ""); diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.h b/storage/src/vespa/storage/bucketdb/storbucketdb.h index fb54ac074d3..77051dc86d7 100644 --- a/storage/src/vespa/storage/bucketdb/storbucketdb.h +++ b/storage/src/vespa/storage/bucketdb/storbucketdb.h @@ -16,10 +16,11 @@ class StorBucketDatabase { std::unique_ptr<bucketdb::AbstractBucketMap<bucketdb::StorageBucketInfo>> _impl; public: using Entry = bucketdb::StorageBucketInfo; - using key_type = bucketdb::AbstractBucketMap<Entry>::key_type; - using Decision = bucketdb::AbstractBucketMap<Entry>::Decision; - using WrappedEntry = bucketdb::AbstractBucketMap<Entry>::WrappedEntry; - using EntryMap = bucketdb::AbstractBucketMap<Entry>::EntryMap; + using BucketMap = bucketdb::AbstractBucketMap<Entry>; + using key_type = BucketMap::key_type; + using Decision = BucketMap::Decision; + using WrappedEntry = BucketMap::WrappedEntry; + using EntryMap = BucketMap::EntryMap; using BucketId = document::BucketId; enum Flag { @@ -29,8 +30,7 @@ public: explicit StorBucketDatabase(const ContentBucketDbOptions&); - void insert(const document::BucketId&, const bucketdb::StorageBucketInfo&, - const char* clientId); + void insert(const document::BucketId&, const Entry&, const char* clientId); bool erase(const document::BucketId&, const char* clientId); @@ -57,16 +57,12 @@ public: * thread between each such such to allow other threads to get a chance * at acquiring a bucket lock. */ - void for_each_chunked(std::function<Decision(uint64_t, const bucketdb::StorageBucketInfo&)> func, - const char* clientId, - vespalib::duration yieldTime = 10us, - uint32_t chunkSize = bucketdb::AbstractBucketMap<bucketdb::StorageBucketInfo>::DEFAULT_CHUNK_SIZE); + void for_each_chunked(std::function<Decision(uint64_t, const Entry &)> func, const char* clientId, + vespalib::duration yieldTime = 10us, uint32_t chunkSize = BucketMap::DEFAULT_CHUNK_SIZE); - void for_each_mutable_unordered(std::function<Decision(uint64_t, bucketdb::StorageBucketInfo&)> func, - const char* clientId); + void for_each_mutable_unordered(std::function<Decision(uint64_t, Entry &)> func, const char* clientId); - void for_each(std::function<Decision(uint64_t, const bucketdb::StorageBucketInfo&)> func, - const char* clientId); + void for_each(std::function<Decision(uint64_t, const Entry &)> func, const char* clientId); [[nodiscard]] std::unique_ptr<bucketdb::ReadGuard<Entry>> acquire_read_guard() const; diff --git a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java index 150021091ca..68af9aa0a49 100644 --- a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java +++ b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java @@ -97,13 +97,18 @@ public abstract class Maintainer implements Runnable { /** * Called once each time this maintenance job should run. * - * @return the degree to which the run was deviated from the successFactorBaseline - a number between -1 (no success), to 0 (complete success). + * @return the degree to which the run successFactor deviated from the successFactorBaseline + * - a number between -1 (no success), to 0 (complete success) measured against the + * successFactorBaseline, or higher if the success factor is higher than the successFactorBaseline. + * The default successFactorBaseline is 1.0. + * If a maintainer is expected to fail sometimes, the successFactorBaseline should be set to a lower value. + * * Note that this indicates whether something is wrong, so e.g. if the call did nothing because it should do * nothing, 0.0 should be returned. */ protected abstract double maintain(); - /** Convenience methods to convert attempts and failures into a success factor, and return */ + /** Convenience methods to convert attempts and failures into a success factor deviation from the baseline, and return */ protected final double asSuccessFactorDeviation(int attempts, int failures) { double factor = attempts == 0 ? 1.0 : 1 - (double)failures / attempts; return new BigDecimal(factor - successFactorBaseline).setScale(2, RoundingMode.HALF_UP).doubleValue(); @@ -117,7 +122,7 @@ public abstract class Maintainer implements Runnable { if (!force && !jobControl.isActive(name())) return; log.log(Level.FINE, () -> "Running " + this.getClass().getSimpleName()); - double successFactorDeviation = 0; + double successFactorDeviation = -1; long startTime = clock.millis(); try (var lock = jobControl.lockJob(name())) { successFactorDeviation = maintain(); diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java index bb62b1189a1..cdb5e36a455 100644 --- a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java +++ b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java @@ -55,7 +55,7 @@ public class MaintainerTest { // Maintainer throws maintainer.throwOnNextRun(new RuntimeException()).run(); - assertEquals(0, jobMetrics.successFactor, delta); + assertEquals(-1, jobMetrics.successFactor, delta); // Maintainer recovers maintainer.throwOnNextRun(null).run(); @@ -64,7 +64,7 @@ public class MaintainerTest { // Lock exception is treated as a failure maintainer.throwOnNextRun(new UncheckedTimeoutException()).run(); - assertEquals(0, jobMetrics.successFactor, delta); + assertEquals(-1, jobMetrics.successFactor, delta); } private static class TestJobMetrics extends JobMetrics { diff --git a/vespalib/src/tests/datastore/datastore/datastore_test.cpp b/vespalib/src/tests/datastore/datastore/datastore_test.cpp index 9e27ed37dd3..fb7a38aacb6 100644 --- a/vespalib/src/tests/datastore/datastore/datastore_test.cpp +++ b/vespalib/src/tests/datastore/datastore/datastore_test.cpp @@ -87,7 +87,7 @@ public: while (sizes.size() < bufs) { RefType iRef = (_type.getArraySize() == 1) ? (_store.template allocator<DataType>(_typeId).alloc().ref) : - (_store.template allocator<DataType>(_typeId).allocArray().ref); + (_store.template allocator<DataType>(_typeId).allocArray(_type.getArraySize()).ref); int bufferId = iRef.bufferId(); if (bufferId != prevBufferId) { if (prevBufferId >= 0) { @@ -126,7 +126,7 @@ public: while (buffers.size() < bufs) { RefType iRef = (_type.getArraySize() == 1) ? (_store.template allocator<DataType>(_typeId).alloc().ref) : - (_store.template allocator<DataType>(_typeId).allocArray().ref); + (_store.template allocator<DataType>(_typeId).allocArray(_type.getArraySize()).ref); int buffer_id = iRef.bufferId(); if (buffers.empty() || buffers.back() != buffer_id) { buffers.push_back(buffer_id); diff --git a/vespalib/src/vespa/vespalib/btree/btreestore.hpp b/vespalib/src/vespa/vespalib/btree/btreestore.hpp index 90c302af5e4..36c47e8bfb0 100644 --- a/vespalib/src/vespa/vespalib/btree/btreestore.hpp +++ b/vespalib/src/vespa/vespalib/btree/btreestore.hpp @@ -74,7 +74,7 @@ allocNewKeyData(uint32_t clusterSize) { assert(clusterSize >= 1 && clusterSize <= clusterLimit); uint32_t typeId = clusterSize - 1; - return _store.allocator<KeyDataType>(typeId).allocArray(); + return _store.allocator<KeyDataType>(typeId).allocArray(clusterSize); } @@ -87,7 +87,7 @@ allocKeyData(uint32_t clusterSize) { assert(clusterSize >= 1 && clusterSize <= clusterLimit); uint32_t typeId = clusterSize - 1; - return _store.freeListAllocator<KeyDataType, datastore::DefaultReclaimer<KeyDataType>>(typeId).allocArray(); + return _store.freeListAllocator<KeyDataType, datastore::DefaultReclaimer<KeyDataType>>(typeId).allocArray(clusterSize); } diff --git a/vespalib/src/vespa/vespalib/datastore/allocator.h b/vespalib/src/vespa/vespalib/datastore/allocator.h index 30938bdc1c1..209ce1a5f26 100644 --- a/vespalib/src/vespa/vespalib/datastore/allocator.h +++ b/vespalib/src/vespa/vespalib/datastore/allocator.h @@ -30,7 +30,7 @@ public: HandleType alloc(Args && ... args); HandleType allocArray(ConstArrayRef array); - HandleType allocArray(); + HandleType allocArray(size_t array_size); }; } diff --git a/vespalib/src/vespa/vespalib/datastore/allocator.hpp b/vespalib/src/vespa/vespalib/datastore/allocator.hpp index fa97ef9a5f5..20b22a032f0 100644 --- a/vespalib/src/vespa/vespalib/datastore/allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/allocator.hpp @@ -52,14 +52,14 @@ Allocator<EntryT, RefT>::allocArray(ConstArrayRef array) template <typename EntryT, typename RefT> typename Allocator<EntryT, RefT>::HandleType -Allocator<EntryT, RefT>::allocArray() +Allocator<EntryT, RefT>::allocArray(size_t array_size) { _store.ensure_buffer_capacity(_typeId, 1); uint32_t buffer_id = _store.primary_buffer_id(_typeId); BufferState &state = _store.getBufferState(buffer_id); assert(state.isActive()); RefT ref(state.size(), buffer_id); - auto array_size = state.getArraySize(); + assert(array_size == state.getArraySize()); EntryT *buf = _store.template getEntryArray<EntryT>(ref, array_size); for (size_t i = 0; i < array_size; ++i) { new (static_cast<void *>(buf + i)) EntryT(); diff --git a/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.cpp b/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.cpp index ff514f5a00b..520fb4cc4ef 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.cpp +++ b/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.cpp @@ -16,11 +16,12 @@ ArrayStoreTypeMapper::~ArrayStoreTypeMapper() = default; uint32_t ArrayStoreTypeMapper::get_type_id(size_t array_size) const { - assert(!_array_sizes.empty()); - auto result = std::lower_bound(_array_sizes.begin() + 1, _array_sizes.end(), array_size); - if (result == _array_sizes.end()) { + assert(_array_sizes.size() >= 2u); + if (array_size > _array_sizes.back()) { return 0; // type id 0 uses buffer type for large arrays } + auto result = std::lower_bound(_array_sizes.begin() + 1, _array_sizes.end(), array_size); + assert(result < _array_sizes.end()); return result - _array_sizes.begin(); } diff --git a/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.h b/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.h index 2a406a39bf9..a73b6ef2e97 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.h +++ b/vespalib/src/vespa/vespalib/datastore/array_store_type_mapper.h @@ -18,7 +18,7 @@ namespace vespalib::datastore { class ArrayStoreTypeMapper { protected: - std::vector<size_t> _array_sizes; + std::vector<uint32_t> _array_sizes; public: ArrayStoreTypeMapper(); ~ArrayStoreTypeMapper(); diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h index dc2d1ea3c34..6d28d76726f 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h +++ b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h @@ -29,7 +29,7 @@ public: HandleType alloc(Args && ... args); HandleType allocArray(ConstArrayRef array); - HandleType allocArray(); + HandleType allocArray(size_t array_size); }; } diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp index 4e69db08a3c..6429f433aaf 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp @@ -83,16 +83,16 @@ FreeListAllocator<EntryT, RefT, ReclaimerT>::allocArray(ConstArrayRef array) template <typename EntryT, typename RefT, typename ReclaimerT> typename Allocator<EntryT, RefT>::HandleType -FreeListAllocator<EntryT, RefT, ReclaimerT>::allocArray() +FreeListAllocator<EntryT, RefT, ReclaimerT>::allocArray(size_t array_size) { auto& free_list = _store.getFreeList(_typeId); if (free_list.empty()) { - return ParentType::allocArray(); + return ParentType::allocArray(array_size); } RefT ref = free_list.pop_entry(); auto& state = _store.getBufferState(ref.bufferId()); - auto size = state.getArraySize(); - EntryT *buf = _store.template getEntryArray<EntryT>(ref, size); + assert(array_size == state.getArraySize()); + EntryT *buf = _store.template getEntryArray<EntryT>(ref, array_size); return HandleType(ref, buf); } |