diff options
80 files changed, 1333 insertions, 412 deletions
diff --git a/bundle-plugin-test/integration-test/pom.xml b/bundle-plugin-test/integration-test/pom.xml new file mode 100644 index 00000000000..42d3b0e4d62 --- /dev/null +++ b/bundle-plugin-test/integration-test/pom.xml @@ -0,0 +1,92 @@ +<?xml version="1.0"?> +<!-- Copyright 2019 Yahoo Holdings. 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>bundle-plugin-test</artifactId> + <version>7-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + <artifactId>integration-test</artifactId> + <version>7-SNAPSHOT</version> + <packaging>jar</packaging> + + <dependencies> + <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>config</artifactId> + <version>${project.version}</version> + <scope>test</scope> + </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>vespajlib</artifactId> + <version>${project.version}</version> + <scope>test</scope> + </dependency> + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <scope>test</scope> + </dependency> + <dependency> + <groupId>org.apache.maven</groupId> + <artifactId>maven-artifact</artifactId> + <scope>test</scope> + </dependency> + + <dependency> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>main</artifactId> + <classifier>bundle</classifier> + <version>${project.version}</version> + </dependency> + <dependency> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>artifact-version-for-exports</artifactId> + <classifier>bundle</classifier> + <version>${project.version}</version> + </dependency> + </dependencies> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-surefire-plugin</artifactId> + <configuration> + <redirectTestOutputToFile>${test.hide}</redirectTestOutputToFile> + <systemPropertyVariables> + <test.bundle.path>${project.build.directory}/dependency</test.bundle.path> + </systemPropertyVariables> + <trimStackTrace>false</trimStackTrace> + </configuration> + </plugin> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-dependency-plugin</artifactId> + <executions> + <execution> + <id>copy-dependencies</id> + <phase>process-test-classes</phase> + <goals> + <goal>copy-dependencies</goal> + </goals> + <configuration> + <includeScope>compile</includeScope> + <excludeTransitive>true</excludeTransitive> + <stripVersion>true</stripVersion> + </configuration> + </execution> + </executions> + </plugin> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + </plugin> + </plugins> + </build> +</project> diff --git a/bundle-plugin-test/src/test/java/com/yahoo/BundleIT.java b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java index 932b78a5c8d..a46abce1dff 100644 --- a/bundle-plugin-test/src/test/java/com/yahoo/BundleIT.java +++ b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java @@ -1,24 +1,29 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo; +package com.yahoo.container.plugin; import com.yahoo.osgi.maven.ProjectBundleClassPaths; import com.yahoo.vespa.config.VespaVersion; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import java.io.File; -import java.io.FilenameFilter; import java.io.IOException; -import java.net.URISyntaxException; -import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import java.util.Collection; +import java.util.Enumeration; 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.zip.ZipEntry; import static com.yahoo.osgi.maven.ProjectBundleClassPaths.CLASSPATH_MAPPINGS_FILENAME; +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; import static org.hamcrest.CoreMatchers.allOf; import static org.hamcrest.CoreMatchers.anyOf; import static org.hamcrest.CoreMatchers.containsString; @@ -26,21 +31,29 @@ import static org.hamcrest.CoreMatchers.endsWith; import static org.hamcrest.CoreMatchers.hasItems; import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; /** * Verifies the bundle jar file built and its manifest. + * * @author Tony Vaagenes */ -public class BundleIT { +public class BundleTest { + static final String TEST_BUNDLE_PATH = System.getProperty("test.bundle.path", ".") + "/"; + + // If bundle-plugin-test is compiled in a mvn command that also built dependencies, e.g. jrt, + // the artifact is jrt.jar, otherwise the installed and versioned artifact + // is used: jrt-7-SNAPSHOT.jar or e.g. jrt-7.123.45.jar. + private static String snapshotOrVersionOrNone = "(-\\d+((-SNAPSHOT)|((\\.\\d+(\\.\\d+)?)?))?)?\\.jar"; + private JarFile jarFile; private Attributes mainAttributes; @Before public void setup() { try { - File componentJar = findBundleJar(); + File componentJar = findBundleJar("main"); jarFile = new JarFile(componentJar); Manifest manifest = jarFile.getManifest(); mainAttributes = manifest.getMainAttributes(); @@ -49,19 +62,13 @@ public class BundleIT { } } - private File findBundleJar() { - File[] componentFile = new File("target").listFiles(new FilenameFilter() { - @Override - public boolean accept(File file, String fileName) { - return fileName.endsWith("-deploy.jar") || fileName.endsWith("-jar-with-dependencies.jar"); - } - }); - - if (componentFile.length != 1) { - throw new RuntimeException("Failed finding component jar file"); + static File findBundleJar(String bundleName) { + Path bundlePath = Paths.get(TEST_BUNDLE_PATH, bundleName + "-bundle.jar"); + if (! Files.exists(bundlePath)) { + throw new RuntimeException("Failed finding component jar file: " + bundlePath); } - return componentFile[0]; + return bundlePath.toFile(); } @Test @@ -75,7 +82,7 @@ public class BundleIT { @Test public void require_that_bundle_symbolic_name_matches_pom_artifactId() { - assertThat(mainAttributes.getValue("Bundle-SymbolicName"), is("bundle-plugin-test")); + assertThat(mainAttributes.getValue("Bundle-SymbolicName"), is("main")); } @Test @@ -115,23 +122,28 @@ public class BundleIT { public void require_that_manifest_contains_bundle_class_path() { String bundleClassPath = mainAttributes.getValue("Bundle-ClassPath"); assertThat(bundleClassPath, containsString(".,")); - // If bundle-plugin-test is compiled in a mvn command that also built jrt, - // the jrt artifact is jrt.jar, otherwise the installed and versioned artifact - // is used: jrt-7-SNAPSHOT.jar. - assertThat(bundleClassPath, anyOf( - containsString("dependencies/jrt-7-SNAPSHOT.jar"), - containsString("dependencies/jrt.jar"))); + + Pattern jrtPattern = Pattern.compile("dependencies/jrt" + snapshotOrVersionOrNone); + assertTrue("Bundle class path did not contain jrt.", jrtPattern.matcher(bundleClassPath).find()); } @Test public void require_that_component_jar_file_contains_compile_artifacts() { - ZipEntry versionedEntry = jarFile.getEntry("dependencies/jrt-7-SNAPSHOT.jar"); - ZipEntry unversionedEntry = jarFile.getEntry("dependencies/jrt.jar"); - if (versionedEntry == null) { - assertNotNull(unversionedEntry); - } else { - assertNull(unversionedEntry); + String depJrt = "dependencies/jrt"; + Pattern jrtPattern = Pattern.compile(depJrt + snapshotOrVersionOrNone); + ZipEntry jrtEntry = null; + + Enumeration<JarEntry> entries = jarFile.entries(); + while (entries.hasMoreElements()) { + var e = entries.nextElement(); + if (e.getName().startsWith(depJrt)) { + if (jrtPattern.matcher(e.getName()).matches()) { + jrtEntry = e; + break; + } + } } + assertNotNull("Component jar file did not contain jrt dependency.", jrtEntry); } @@ -141,17 +153,25 @@ public class BundleIT { assertThat(webInfUrl, containsString("/WEB-INF/web.xml")); } + // TODO Vespa 8: Remove, the classpath mappings file is only needed for jersey resources to work in the application test framework. + // When this test is removed, also remove the maven-resources-plugin from the 'main' test bundle's pom. + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); @SuppressWarnings("unchecked") @Test - public void bundle_class_path_mappings_are_generated() throws URISyntaxException, IOException { - URL mappingsUrl = getClass().getResource("/" + CLASSPATH_MAPPINGS_FILENAME); + public void bundle_class_path_mappings_are_generated() throws Exception { + ZipEntry classpathMappingsEntry = jarFile.getEntry(CLASSPATH_MAPPINGS_FILENAME); + assertNotNull( - "Could not find " + CLASSPATH_MAPPINGS_FILENAME + " in the test output directory", - mappingsUrl); + "Could not find " + CLASSPATH_MAPPINGS_FILENAME + " in the test bundle", + classpathMappingsEntry); + + Path mappingsFile = tempFolder.newFile(CLASSPATH_MAPPINGS_FILENAME).toPath(); + Files.copy(jarFile.getInputStream(classpathMappingsEntry), mappingsFile, REPLACE_EXISTING); - ProjectBundleClassPaths bundleClassPaths = ProjectBundleClassPaths.load(Paths.get(mappingsUrl.toURI())); + ProjectBundleClassPaths bundleClassPaths = ProjectBundleClassPaths.load(mappingsFile); - assertThat(bundleClassPaths.mainBundle.bundleSymbolicName, is("bundle-plugin-test")); + assertThat(bundleClassPaths.mainBundle.bundleSymbolicName, is("main")); Collection<String> mainBundleClassPaths = bundleClassPaths.mainBundle.classPathElements; @@ -159,7 +179,7 @@ public class BundleIT { hasItems( endsWith("target/classes"), anyOf( - allOf(containsString("jrt"), containsString(".jar"), containsString("m2/repository")), - containsString("jrt/target/jrt.jar")))); + allOf(containsString("/jrt-"), containsString(".jar")), + containsString("/jrt.jar")))); } } diff --git a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/ExportPackageVersionTest.java b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/ExportPackageVersionTest.java new file mode 100644 index 00000000000..66f36e32f39 --- /dev/null +++ b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/ExportPackageVersionTest.java @@ -0,0 +1,72 @@ +// Copyright 2019 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.container.plugin; + +import org.junit.BeforeClass; +import org.junit.Test; + +import java.io.File; +import java.io.IOException; +import java.util.jar.Attributes; +import java.util.jar.JarFile; + +import static com.yahoo.container.plugin.BundleTest.findBundleJar; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +/** + * Verifies that the 'useArtifactVersionForExportPackages' setting for the bundle-plugin works as intended. + * + * @author gjoranv + */ +public class ExportPackageVersionTest { + + private static Attributes mainAttributes; + private static String bundleVersion; + + @BeforeClass + public static void setup() { + try { + File componentJar = findBundleJar("artifact-version-for-exports"); + mainAttributes = new JarFile(componentJar).getManifest().getMainAttributes(); + bundleVersion = mainAttributes.getValue("Bundle-Version"); + + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @Test + public void artifact_version_without_qualifier_is_used_as_export_version() { + // Bundle-Version is artifact version without qualifier + String expectedExport = "ai.vespa.noversion;version=" + bundleVersion; + + String exportPackage = mainAttributes.getValue("Export-Package"); + assertThat(exportPackage, containsString(expectedExport)); + + // Verify that there is no qualifier + assertThat(exportPackage, not(containsString(expectedExport + "."))); + } + + @Test + public void explicit_version_in_ExportPackage_annotation_overrides_artifact_version() { + String exportPackage = mainAttributes.getValue("Export-Package"); + assertThat(exportPackage, containsString("ai.vespa.explicitversion;version=2.4.6.RELEASE")); + } + + @Test + public void artifact_version_of_dependency_is_used_as_export_version_for_package_in_compile_scoped_dependency() { + String exportPackage = mainAttributes.getValue("Export-Package"); + + // TODO: This test should have checked for a fixed version of the dependency bundle, different than the main bundle version. + // See comment in the dependency bundle's pom.xml for why this is not the case. + assertThat(exportPackage, containsString("ai.vespa.noversion_dep;version=" + bundleVersion)); + } + + @Test + public void explicit_version_in_ExportPackage_annotation_overrides_artifact_version_of_compile_scoped_dependency() { + String exportPackage = mainAttributes.getValue("Export-Package"); + assertThat(exportPackage, containsString("ai.vespa.explicitversion_dep;version=3.6.9.RELEASE")); + } + +} diff --git a/bundle-plugin-test/pom.xml b/bundle-plugin-test/pom.xml index 3ba9bf7b4b6..b27f6edc5f8 100644 --- a/bundle-plugin-test/pom.xml +++ b/bundle-plugin-test/pom.xml @@ -11,80 +11,12 @@ <version>7-SNAPSHOT</version> <relativePath>../parent/pom.xml</relativePath> </parent> - <groupId>com.yahoo.vespa</groupId> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> <artifactId>bundle-plugin-test</artifactId> <version>7-SNAPSHOT</version> - <packaging>container-plugin</packaging> - <dependencies> - <dependency> - <groupId>junit</groupId> - <artifactId>junit</artifactId> - <scope>test</scope> - </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>container-dev</artifactId> - <version>${project.version}</version> - <scope>provided</scope> - </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>vespajlib</artifactId> - <version>${project.version}</version> - <scope>provided</scope> - </dependency> - <dependency> - <groupId>org.json</groupId> - <artifactId>json</artifactId> - <scope>provided</scope> - </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>jrt</artifactId> - <version>${project.version}</version> - </dependency> - <dependency> - <!-- Added to verify that module-info.class can be handled by bundle-plugin without throwing an exception. --> - <groupId>javax.xml.bind</groupId> - <artifactId>jaxb-api</artifactId> - <version>2.3.0</version> - </dependency> - </dependencies> - <build> - <plugins> - <plugin> - <groupId>com.yahoo.vespa</groupId> - <artifactId>bundle-plugin</artifactId> - <version>${project.version}</version> - <extensions>true</extensions> - <configuration> - <Import-Package> - manualImport.withoutVersion, - manualImport.withVersion;version="12.3.4", - multiple.packages.with.the.same.version1;multiple.packages.with.the.same.version2;version="[1,2)" - </Import-Package> - <WebInfUrl>/WEB-INF/web.xml</WebInfUrl> - </configuration> - </plugin> - <plugin> - <groupId>org.apache.maven.plugins</groupId> - <artifactId>maven-failsafe-plugin</artifactId> - <version>2.9</version> - <executions> - <execution> - <id>integration-test</id> - <goals> - <goal>integration-test</goal> - </goals> - </execution> - <execution> - <id>verify</id> - <goals> - <goal>verify</goal> - </goals> - </execution> - </executions> - </plugin> - </plugins> - </build> + <packaging>pom</packaging> + <modules> + <module>integration-test</module> + <module>test-bundles</module> + </modules> </project> diff --git a/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/pom.xml b/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/pom.xml new file mode 100644 index 00000000000..34b250ae927 --- /dev/null +++ b/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/pom.xml @@ -0,0 +1,37 @@ +<?xml version="1.0"?> +<!-- Copyright 2019 Yahoo Holdings. 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>7-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + <artifactId>artifact-version-for-exports-dep</artifactId> + <!-- TODO: Should use a fixed version different than the dependent bundle. + But version is set to the release version by build scripts before building. + Then, the dependent bundle will not find the artifact. Skipping this step for a sub-module seems + impossible with the maven-versions-plugin, and cumbersome with factorylib. --> + <version>7-SNAPSHOT</version> + <packaging>container-plugin</packaging> + <dependencies> + </dependencies> + <build> + <plugins> + <plugin> + <groupId>com.yahoo.vespa</groupId> + <artifactId>bundle-plugin</artifactId> + <version>${project.version}</version> + <extensions>true</extensions> + <configuration> + <useArtifactVersionForExportPackages>true</useArtifactVersionForExportPackages> + <configGenVersion>${project.version}</configGenVersion> + </configuration> + </plugin> + </plugins> + </build> +</project> diff --git a/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/src/main/java/ai/vespa/explicitversion_dep/package-info.java b/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/src/main/java/ai/vespa/explicitversion_dep/package-info.java new file mode 100644 index 00000000000..3eea8b11f1e --- /dev/null +++ b/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/src/main/java/ai/vespa/explicitversion_dep/package-info.java @@ -0,0 +1,6 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage(version = @Version(major = 3, minor = 6, micro = 9, qualifier = "RELEASE")) +package ai.vespa.explicitversion_dep; + +import com.yahoo.osgi.annotation.ExportPackage; +import com.yahoo.osgi.annotation.Version; diff --git a/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/src/main/java/ai/vespa/noversion_dep/package-info.java b/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/src/main/java/ai/vespa/noversion_dep/package-info.java new file mode 100644 index 00000000000..794e177f7e7 --- /dev/null +++ b/bundle-plugin-test/test-bundles/artifact-version-for-exports-dep/src/main/java/ai/vespa/noversion_dep/package-info.java @@ -0,0 +1,5 @@ +// Copyright 2019 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +package ai.vespa.noversion_dep; + +import com.yahoo.osgi.annotation.ExportPackage; diff --git a/bundle-plugin-test/test-bundles/artifact-version-for-exports/pom.xml b/bundle-plugin-test/test-bundles/artifact-version-for-exports/pom.xml new file mode 100644 index 00000000000..619189cd874 --- /dev/null +++ b/bundle-plugin-test/test-bundles/artifact-version-for-exports/pom.xml @@ -0,0 +1,36 @@ +<?xml version="1.0"?> +<!-- Copyright 2019 Yahoo Holdings. 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>7-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + <artifactId>artifact-version-for-exports</artifactId> + <version>7-SNAPSHOT</version> + <packaging>container-plugin</packaging> + <dependencies> + <dependency> + <groupId>com.yahoo.vespa.bundle-plugin</groupId> + <artifactId>artifact-version-for-exports-dep</artifactId> + <version>${project.version}</version> + </dependency> + </dependencies> + <build> + <plugins> + <plugin> + <groupId>com.yahoo.vespa</groupId> + <artifactId>bundle-plugin</artifactId> + <extensions>true</extensions> + <configuration> + <useArtifactVersionForExportPackages>true</useArtifactVersionForExportPackages> + </configuration> + </plugin> + </plugins> + </build> +</project> diff --git a/bundle-plugin-test/test-bundles/artifact-version-for-exports/src/main/java/ai/vespa/explicitversion/package-info.java b/bundle-plugin-test/test-bundles/artifact-version-for-exports/src/main/java/ai/vespa/explicitversion/package-info.java new file mode 100644 index 00000000000..96cf6a3bf85 --- /dev/null +++ b/bundle-plugin-test/test-bundles/artifact-version-for-exports/src/main/java/ai/vespa/explicitversion/package-info.java @@ -0,0 +1,6 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage(version = @Version(major = 2, minor = 4, micro = 6, qualifier = "RELEASE")) +package ai.vespa.explicitversion; + +import com.yahoo.osgi.annotation.ExportPackage; +import com.yahoo.osgi.annotation.Version; diff --git a/bundle-plugin-test/test-bundles/artifact-version-for-exports/src/main/java/ai/vespa/noversion/package-info.java b/bundle-plugin-test/test-bundles/artifact-version-for-exports/src/main/java/ai/vespa/noversion/package-info.java new file mode 100644 index 00000000000..e7e11e39889 --- /dev/null +++ b/bundle-plugin-test/test-bundles/artifact-version-for-exports/src/main/java/ai/vespa/noversion/package-info.java @@ -0,0 +1,5 @@ +// Copyright 2019 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +package ai.vespa.noversion; + +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 new file mode 100644 index 00000000000..c9c9ea270eb --- /dev/null +++ b/bundle-plugin-test/test-bundles/main/pom.xml @@ -0,0 +1,78 @@ +<?xml version="1.0"?> +<!-- Copyright 2019 Yahoo Holdings. 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>7-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + <artifactId>main</artifactId> + <version>7-SNAPSHOT</version> + <packaging>container-plugin</packaging> + <dependencies> + <dependency> + <groupId>org.json</groupId> + <artifactId>json</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>jrt</artifactId> + <version>${project.version}</version> + </dependency> + <dependency> + <!-- Added to verify that module-info.class can be handled by bundle-plugin without throwing an exception. --> + <groupId>javax.xml.bind</groupId> + <artifactId>jaxb-api</artifactId> + <version>2.3.0</version> + </dependency> + </dependencies> + <build> + <plugins> + <plugin> + <groupId>com.yahoo.vespa</groupId> + <artifactId>bundle-plugin</artifactId> + <extensions>true</extensions> + <configuration> + <Import-Package> + manualImport.withoutVersion, + manualImport.withVersion;version="12.3.4", + multiple.packages.with.the.same.version1;multiple.packages.with.the.same.version2;version="[1,2)" + </Import-Package> + <WebInfUrl>/WEB-INF/web.xml</WebInfUrl> + </configuration> + </plugin> + <plugin> + <!-- Trick to copy bundle-plugin.bundle-classpath-mappings.json from target/test-classes to target/classes --> + <artifactId>maven-resources-plugin</artifactId> + <executions> + <execution> + <id>copy-bundle-classpath-mappings-from-test-to-main</id> + <!-- NOTE: Must be done after generating classpath-mappings and before assembling the bundle (see the test-bundles pom) --> + <phase>process-test-sources</phase> + <goals> + <goal>copy-resources</goal> + </goals> + <configuration> + <outputDirectory>${project.build.outputDirectory}</outputDirectory> + <overwrite>true</overwrite> + <resources> + <resource> + <directory>${project.build.testOutputDirectory}</directory> + <includes> + <include>bundle-plugin.bundle-classpath-mappings.json</include> + </includes> + </resource> + </resources> + </configuration> + </execution> + </executions> + </plugin> + </plugins> + </build> +</project> diff --git a/bundle-plugin-test/src/main/java/InDefaultPackage.java b/bundle-plugin-test/test-bundles/main/src/main/java/InDefaultPackage.java index a650916d653..a650916d653 100644 --- a/bundle-plugin-test/src/main/java/InDefaultPackage.java +++ b/bundle-plugin-test/test-bundles/main/src/main/java/InDefaultPackage.java diff --git a/bundle-plugin-test/src/main/java/com/yahoo/test/SimpleSearcher.java b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/test/SimpleSearcher.java index dddca3f4d59..dddca3f4d59 100644 --- a/bundle-plugin-test/src/main/java/com/yahoo/test/SimpleSearcher.java +++ b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/test/SimpleSearcher.java diff --git a/bundle-plugin-test/src/main/java/com/yahoo/test/SimpleSearcher2.java b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/test/SimpleSearcher2.java index 3220171de13..3220171de13 100644 --- a/bundle-plugin-test/src/main/java/com/yahoo/test/SimpleSearcher2.java +++ b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/test/SimpleSearcher2.java diff --git a/bundle-plugin-test/src/main/java/com/yahoo/test/package-info.java b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/test/package-info.java index 5774fc8d5f2..5774fc8d5f2 100644 --- a/bundle-plugin-test/src/main/java/com/yahoo/test/package-info.java +++ b/bundle-plugin-test/test-bundles/main/src/main/java/com/yahoo/test/package-info.java diff --git a/bundle-plugin-test/src/main/resources/configdefinitions/test.def b/bundle-plugin-test/test-bundles/main/src/main/resources/configdefinitions/test.def index b4ba9ec518a..b4ba9ec518a 100644 --- a/bundle-plugin-test/src/main/resources/configdefinitions/test.def +++ b/bundle-plugin-test/test-bundles/main/src/main/resources/configdefinitions/test.def diff --git a/bundle-plugin-test/test-bundles/pom.xml b/bundle-plugin-test/test-bundles/pom.xml new file mode 100644 index 00000000000..712ccb5542e --- /dev/null +++ b/bundle-plugin-test/test-bundles/pom.xml @@ -0,0 +1,75 @@ +<?xml version="1.0"?> +<!-- Copyright 2019 Yahoo Holdings. 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>bundle-plugin-test</artifactId> + <version>7-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + <artifactId>test-bundles</artifactId> + <version>7-SNAPSHOT</version> + <packaging>pom</packaging> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + </plugin> + </plugins> + <pluginManagement> + <plugins> + <plugin> + <!-- Trick to package bundles before test phase to allow running 'mvn test' --> + <groupId>com.yahoo.vespa</groupId> + <artifactId>bundle-plugin</artifactId> + <version>${project.version}</version> + <executions> + <execution> + <id>generate-classpath-mappings</id> + <phase>generate-test-sources</phase> + <goals> + <goal>generate-bundle-classpath-mappings</goal> + </goals> + </execution> + <execution> + <id>package-test-bundles</id> + <!-- Must be done after generating classpath-mappings and copying it in the 'main' bundle. --> + <phase>test-compile</phase> + <goals> + <goal>generate-osgi-manifest</goal> + <goal>assemble-container-plugin</goal> + </goals> + </execution> + </executions> + <configuration> + <!-- Make the integration-test module use the bundle jars instead of the ordinary artifacts. --> + <AttachBundle>true</AttachBundle> + </configuration> + </plugin> + </plugins> + </pluginManagement> + </build> + <modules> + <module>artifact-version-for-exports</module> + <module>artifact-version-for-exports-dep</module> + <module>main</module> + </modules> + + <dependencies> + <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>container-dev</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + </dependencies> + + <properties> + <maven.javadoc.skip>true</maven.javadoc.skip> + </properties> +</project> 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 13a306a0c1c..d586f324f77 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 @@ -1,17 +1,18 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.container.plugin.classanalysis; +import org.apache.maven.artifact.versioning.ArtifactVersion; import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.ClassReader; import org.objectweb.asm.Opcodes; import org.objectweb.asm.Type; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.util.Optional; -import static com.yahoo.container.plugin.util.IO.withFileInputStream; /** * Main entry point for class analysis @@ -21,16 +22,20 @@ import static com.yahoo.container.plugin.util.IO.withFileInputStream; */ public class Analyze { public static ClassFileMetaData analyzeClass(File classFile) { + return analyzeClass(classFile, null); + } + + public static ClassFileMetaData analyzeClass(File classFile, ArtifactVersion artifactVersion) { try { - return withFileInputStream(classFile, Analyze::analyzeClass); - } catch (RuntimeException e) { + return analyzeClass(new FileInputStream(classFile), artifactVersion); + } catch (Exception e) { throw new RuntimeException("An error occurred when analyzing " + classFile.getPath(), e); } } - public static ClassFileMetaData analyzeClass(InputStream inputStream) { + public static ClassFileMetaData analyzeClass(InputStream inputStream, ArtifactVersion artifactVersion) { try { - AnalyzeClassVisitor visitor = new AnalyzeClassVisitor(); + AnalyzeClassVisitor visitor = new AnalyzeClassVisitor(artifactVersion); new ClassReader(inputStream).accept(visitor, ClassReader.SKIP_DEBUG); return visitor.result(); } catch (IOException e) { 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 0225c43e4b8..e37a9caa7cc 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 @@ -3,6 +3,7 @@ package com.yahoo.container.plugin.classanalysis; import com.yahoo.osgi.annotation.ExportPackage; import com.yahoo.osgi.annotation.Version; +import org.apache.maven.artifact.versioning.ArtifactVersion; import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.Attribute; import org.objectweb.asm.ClassVisitor; @@ -27,8 +28,11 @@ class AnalyzeClassVisitor extends ClassVisitor implements ImportCollector { private Set<String> imports = new HashSet<>(); private Optional<ExportPackageAnnotation> exportPackageAnnotation = Optional.empty(); - AnalyzeClassVisitor() { + private final Optional<ArtifactVersion> defaultExportPackageVersion; + + AnalyzeClassVisitor(ArtifactVersion defaultExportPackageVersion) { super(Opcodes.ASM7); + this.defaultExportPackageVersion = Optional.ofNullable(defaultExportPackageVersion); } @Override @@ -99,9 +103,14 @@ class AnalyzeClassVisitor extends ClassVisitor implements ImportCollector { private AnnotationVisitor visitExportPackage() { return new AnnotationVisitor(Opcodes.ASM7) { - private int major = defaultVersionValue("major"); - private int minor = defaultVersionValue("minor"); - private int micro = defaultVersionValue("micro"); + private int major = defaultExportPackageVersion.map(ArtifactVersion::getMajorVersion) + .orElse(defaultVersionValue("major")); + private int minor = defaultExportPackageVersion.map(ArtifactVersion::getMinorVersion) + .orElse(defaultVersionValue("minor")); + private int micro = defaultExportPackageVersion.map(ArtifactVersion::getIncrementalVersion) + .orElse(defaultVersionValue("micro")); + + // Default qualifier is the empty string. private String qualifier = defaultVersionValue("qualifier"); @Override 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 c23e55a9eec..11a82ab7443 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 @@ -14,6 +14,8 @@ import com.yahoo.container.plugin.osgi.ImportPackages.Import; import com.yahoo.container.plugin.util.Strings; import org.apache.commons.lang3.tuple.Pair; import org.apache.maven.artifact.Artifact; +import org.apache.maven.artifact.versioning.ArtifactVersion; +import org.apache.maven.artifact.versioning.DefaultArtifactVersion; import org.apache.maven.plugin.AbstractMojo; import org.apache.maven.plugin.MojoExecutionException; import org.apache.maven.plugins.annotations.Mojo; @@ -44,8 +46,7 @@ import static com.yahoo.container.plugin.osgi.ExportPackages.exportsByPackageNam import static com.yahoo.container.plugin.osgi.ImportPackages.calculateImports; import static com.yahoo.container.plugin.util.Files.allDescendantFiles; import static com.yahoo.container.plugin.util.IO.withFileOutputStream; -import static com.yahoo.container.plugin.util.JarFiles.withInputStream; -import static com.yahoo.container.plugin.util.JarFiles.withJarFile; + /** * @author Tony Vaagenes @@ -57,6 +58,15 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { @Parameter(defaultValue = "${project}") private MavenProject project = null; + /** + * If set to true, the artifact's version is used as default package version for ExportPackages. + * Packages from included (compile scoped) artifacts will use the version for their own artifact. + * If the package is exported with an explicit version in package-info.java, that version will be + * used regardless of this parameter. + */ + @Parameter(alias = "UseArtifactVersionForExportPackages", defaultValue = "false") + private boolean useArtifactVersionForExportPackages; + @Parameter private String discApplicationClass = null; @@ -215,8 +225,11 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { Map<String, Optional<String>> manualImports, Collection<Import> imports, PackageTally pluginPackageTally) { Map<String, String> ret = new HashMap<>(); String importPackage = Stream.concat(manualImports.entrySet().stream().map(e -> asOsgiImport(e.getKey(), e.getValue())), - imports.stream().map(Import::asOsgiImport)).sorted().collect(Collectors.joining(",")); - String exportPackage = osgiExportPackages(pluginPackageTally.exportedPackages()).stream().sorted().collect(Collectors.joining(",")); + imports.stream().map(Import::asOsgiImport)).sorted() + .collect(Collectors.joining(",")); + + String exportPackage = osgiExportPackages(pluginPackageTally.exportedPackages()).stream().sorted() + .collect(Collectors.joining(",")); for (Pair<String, String> element : Arrays.asList(// Pair.of("Created-By", "vespa container maven plugin"), // @@ -301,34 +314,47 @@ public class GenerateOsgiManifestMojo extends AbstractMojo { artifact.getId(), artifact.getType()))); } + private ArtifactVersion artifactVersionOrNull(String version) { + return useArtifactVersionForExportPackages ? new DefaultArtifactVersion(version) : null; + } + private PackageTally getProjectClassesTally() { File outputDirectory = new File(project.getBuild().getOutputDirectory()); - List<ClassFileMetaData> analyzedClasses = allDescendantFiles(outputDirectory).filter(file -> file.getName().endsWith(".class")) - .map(Analyze::analyzeClass).collect(Collectors.toList()); + List<ClassFileMetaData> analyzedClasses = allDescendantFiles(outputDirectory) + .filter(file -> file.getName().endsWith(".class")) + .map(classFile -> Analyze.analyzeClass(classFile, artifactVersionOrNull(bundleVersion))) + .collect(Collectors.toList()); return PackageTally.fromAnalyzedClassFiles(analyzedClasses); } - private static PackageTally definedPackages(Collection<Artifact> jarArtifacts) { - return PackageTally.combine(jarArtifacts.stream().map(ja -> withJarFile(ja.getFile(), GenerateOsgiManifestMojo::definedPackages)) - .collect(Collectors.toList())); + private PackageTally definedPackages(Collection<Artifact> jarArtifacts) { + List<PackageTally> tallies = new ArrayList<>(); + for (var artifact : jarArtifacts) { + try { + tallies.add(definedPackages(new JarFile(artifact.getFile()), artifactVersionOrNull(artifact.getVersion()))); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + return PackageTally.combine(tallies); } - private static PackageTally definedPackages(JarFile jarFile) throws MojoExecutionException { + private static PackageTally definedPackages(JarFile jarFile, ArtifactVersion version) throws MojoExecutionException { List<ClassFileMetaData> analyzedClasses = new ArrayList<>(); for (Enumeration<JarEntry> entries = jarFile.entries(); entries.hasMoreElements();) { JarEntry entry = entries.nextElement(); if (! entry.isDirectory() && entry.getName().endsWith(".class")) { - analyzedClasses.add(analyzeClass(jarFile, entry)); + analyzedClasses.add(analyzeClass(jarFile, entry, version)); } } return PackageTally.fromAnalyzedClassFiles(analyzedClasses); } - private static ClassFileMetaData analyzeClass(JarFile jarFile, JarEntry entry) throws MojoExecutionException { + private static ClassFileMetaData analyzeClass(JarFile jarFile, JarEntry entry, ArtifactVersion version) throws MojoExecutionException { try { - return withInputStream(jarFile, entry, Analyze::analyzeClass); + return Analyze.analyzeClass(jarFile.getInputStream(entry), version); } catch (Exception e) { throw new MojoExecutionException( String.format("While analyzing the class '%s' in jar file '%s'", entry.getName(), jarFile.getName()), e); diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/IO.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/IO.java index a1e313b920b..654fb700a43 100644 --- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/IO.java +++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/util/IO.java @@ -13,13 +13,6 @@ import java.io.OutputStream; * @author ollivir */ public class IO { - public static <T> T withFileInputStream(File file, ThrowingFunction<FileInputStream, T> f) { - try (FileInputStream fis = new FileInputStream(file)) { - return f.apply(fis); - } catch (Exception e) { - throw new RuntimeException(e); - } - } /** * Creates a new file and all its parent directories, and provides a file output stream to the file. diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java index 1748e3be64f..e19903ddeda 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java @@ -77,20 +77,20 @@ public class JRTConfigRequester implements RequestWaiter { */ public <T extends ConfigInstance> void request(JRTConfigSubscription<T> sub) { JRTClientConfigRequest req = JRTConfigRequestFactory.createFromSub(sub); - doRequest(sub, req, timingValues.getSubscribeTimeout()); + doRequest(sub, req); } - private <T extends ConfigInstance> void doRequest(JRTConfigSubscription<T> sub, JRTClientConfigRequest req, long timeout) { - com.yahoo.vespa.config.Connection connection = connectionPool.getCurrent(); + private <T extends ConfigInstance> void doRequest(JRTConfigSubscription<T> sub, JRTClientConfigRequest req) { + Connection connection = connectionPool.getCurrent(); req.getRequest().setContext(new RequestContext(sub, req, connection)); - boolean reqOK = req.validateParameters(); - if (!reqOK) throw new ConfigurationRuntimeException("Error in parameters for config request: " + req); - // Add some time to the timeout, we never want it to time out in JRT during normal operation - double jrtClientTimeout = getClientTimeout(timeout); - log.log(LogLevel.DEBUG, () -> "Requesting config for " + sub + " on connection " + connection - + " with RPC timeout " + jrtClientTimeout + - (log.isLoggable(LogLevel.SPAM) ? (",defcontent=" + req.getDefContent().asString()) : "")); + if ( ! req.validateParameters()) throw new ConfigurationRuntimeException("Error in parameters for config request: " + req); + double jrtClientTimeout = getClientTimeout(req); + if (log.isLoggable(LogLevel.DEBUG)) { + log.log(LogLevel.DEBUG, "Requesting config for " + sub + " on connection " + connection + + " with client timeout " + jrtClientTimeout + + (log.isLoggable(LogLevel.SPAM) ? (",defcontent=" + req.getDefContent().asString()) : "")); + } connection.invokeAsync(req.getRequest(), jrtClientTimeout, this); } @@ -174,6 +174,7 @@ public class JRTConfigRequester implements RequestWaiter { delay = timingValues.getConfiguredErrorDelay(); else delay = timingValues.getUnconfiguredDelay(); + if (errorCode == ErrorType.TRANSIENT) { delay = delay * Math.min((transientFailures + 1), timingValues.getMaxDelayMultiplier()); } else { @@ -274,7 +275,7 @@ public class JRTConfigRequester implements RequestWaiter { } public void run() { - doRequest(sub, jrtReq, jrtReq.getTimeout()); + doRequest(sub, jrtReq); } } @@ -323,7 +324,7 @@ public class JRTConfigRequester implements RequestWaiter { return connectionPool; } - private Double getClientTimeout(long serverTimeout) { - return (serverTimeout / 1000.0) + additionalTimeForClientTimeout; + private Double getClientTimeout(JRTClientConfigRequest request) { + return (request.getTimeout() / 1000.0) + additionalTimeForClientTimeout; } } diff --git a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java index 7460e42c866..150fcc502d4 100644 --- a/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java +++ b/configserver-flags/src/test/java/com/yahoo/vespa/configserver/flags/db/FlagsDbImplTest.java @@ -8,6 +8,7 @@ import com.yahoo.vespa.flags.JsonNodeRawFlag; import com.yahoo.vespa.flags.json.Condition; import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.flags.json.Rule; +import com.yahoo.vespa.flags.json.WhitelistCondition; import org.junit.Test; import java.util.Map; @@ -29,7 +30,8 @@ public class FlagsDbImplTest { MockCurator curator = new MockCurator(); FlagsDbImpl db = new FlagsDbImpl(curator); - Condition condition1 = new Condition(Condition.Type.WHITELIST, FetchVector.Dimension.HOSTNAME, "host1"); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).withValues("host1"); + Condition condition1 = WhitelistCondition.create(params); Rule rule1 = new Rule(Optional.of(JsonNodeRawFlag.fromJson("13")), condition1); FlagId flagId = new FlagId("id"); FlagData data = new FlagData(flagId, new FetchVector().with(FetchVector.Dimension.ZONE_ID, "zone-a"), rule1); 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 3eaebf39598..c4f394d237b 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 @@ -15,6 +15,7 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.LogEntry; import com.yahoo.vespa.hosted.controller.api.integration.configserver.NotFoundException; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; +import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevision; @@ -228,6 +229,11 @@ public class JobController { } /** Returns the last run of the given type, for the given application, if one has been run. */ + public Optional<Run> last(JobId job) { + return curator.readLastRun(job.application(), job.type()); + } + + /** Returns the last run of the given type, for the given application, if one has been run. */ public Optional<Run> last(ApplicationId id, JobType type) { return curator.readLastRun(id, type); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java index 73b135aa912..c89d74864cb 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java @@ -116,7 +116,18 @@ public class Upgrader extends Maintainer { /** Returns the number of applications to upgrade in this run */ private int numberOfApplicationsToUpgrade() { - return Math.max(1, (int) (maintenanceInterval().getSeconds() * (upgradesPerMinute() / 60))); + return numberOfApplicationsToUpgrade(maintenanceInterval().dividedBy(Math.max(1, controller().curator().cluster().size())).toMillis(), + controller().clock().millis(), + upgradesPerMinute()); + } + + /** Returns the number of applications to upgrade in the interval containing now */ + static int numberOfApplicationsToUpgrade(long intervalMillis, long nowMillis, double upgradesPerMinute) { + long intervalStart = Math.round(nowMillis / (double) intervalMillis) * intervalMillis; + double upgradesPerMilli = upgradesPerMinute / 60_000; + long upgradesAtStart = (long) (intervalStart * upgradesPerMilli); + long upgradesAtEnd = (long) ((intervalStart + intervalMillis) * upgradesPerMilli); + return (int) (upgradesAtEnd - upgradesAtStart); } /** Returns number of upgrades per minute */ diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index 61b393efbff..f9639835cf0 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -75,6 +75,7 @@ public class DeploymentTester { JobControl jobControl = new JobControl(tester.curator()); this.upgrader = new Upgrader(tester.controller(), maintenanceInterval, jobControl, tester.curator()); + this.upgrader.setUpgradesPerMinute(1); // Anything that makes it at least one for any maintenance period is fine. this.outstandingChangeDeployer = new OutstandingChangeDeployer(tester.controller(), maintenanceInterval, jobControl); this.readyJobTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval, jobControl); this.nameServiceDispatcher = new NameServiceDispatcher(tester.controller(), Duration.ofHours(12), diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java index 46666144153..a5eac44dbc4 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java @@ -18,6 +18,7 @@ import com.yahoo.vespa.hosted.controller.Instance; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.athenz.AthenzDbMock; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; +import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevision; @@ -172,7 +173,7 @@ public class InternalDeploymentTester { while ( ! (activeRuns = jobs().active(id)).isEmpty()) for (Run run : activeRuns) if (jobs.add(run.id().type())) { - runJob(run.id().type()); + runJob(run.id().job()); tester.readyJobTrigger().run(); } else @@ -231,11 +232,10 @@ public class InternalDeploymentTester { } /** Returns the current run for the given job type, and verifies it is still running normally. */ - public Run currentRun(JobType type) { - Run run = jobs.active().stream() - .filter(r -> r.id().type() == type) - .findAny() - .orElseThrow(() -> new AssertionError(type + " is not among the active: " + jobs.active())); + public Run currentRun(JobId job) { + Run run = jobs.last(job) + .filter(r -> r.id().type() == job.type()) + .orElseThrow(() -> new AssertionError(job.type() + " is not among the active: " + jobs.active())); assertFalse(run.hasFailed()); assertNotEquals(aborted, run.status()); return run; @@ -243,49 +243,79 @@ public class InternalDeploymentTester { /** Deploys tester and real app, and completes initial staging installation first if needed. */ public void doDeploy(JobType type) { - RunId id = currentRun(type).id(); - ZoneId zone = type.zone(tester.controller().system()); - DeploymentId deployment = new DeploymentId(instanceId, zone); + doDeploy(new JobId(instanceId, type)); + } + + /** Deploys tester and real app, and completes initial staging installation first if needed. */ + public void doDeploy(ApplicationId instanceId, JobType type) { + doDeploy(new JobId(instanceId, type)); + } + + /** Deploys tester and real app, and completes initial staging installation first if needed. */ + public void doDeploy(JobId job) { + RunId id = currentRun(job).id(); + ZoneId zone = job.type().zone(tester.controller().system()); + DeploymentId deployment = new DeploymentId(job.application(), zone); // First steps are always deployments. - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); - if (type == JobType.stagingTest) { // Do the initial deployment and installation of the real application. + if (job.type() == JobType.stagingTest) { // Do the initial deployment and installation of the real application. assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installInitialReal)); - currentRun(type).versions().sourcePlatform().ifPresent(version -> tester.configServer().nodeRepository().doUpgrade(deployment, Optional.empty(), version)); + currentRun(job).versions().sourcePlatform().ifPresent(version -> tester.configServer().nodeRepository().doUpgrade(deployment, Optional.empty(), version)); tester.configServer().convergeServices(instanceId, zone); setEndpoints(instanceId, zone); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installInitialReal)); } } /** Upgrades nodes to target version. */ public void doUpgrade(JobType type) { - RunId id = currentRun(type).id(); - ZoneId zone = type.zone(tester.controller().system()); - DeploymentId deployment = new DeploymentId(instanceId, zone); + doUpgrade(new JobId(instanceId, type)); + } + + /** Upgrades nodes to target version. */ + public void doUpgrade(ApplicationId instanceId, JobType type) { + doUpgrade(new JobId(instanceId, type)); + } + + /** Upgrades nodes to target version. */ + public void doUpgrade(JobId job) { + RunId id = currentRun(job).id(); + ZoneId zone = job.type().zone(tester.controller().system()); + DeploymentId deployment = new DeploymentId(job.application(), zone); assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installReal)); - tester.configServer().nodeRepository().doUpgrade(deployment, Optional.empty(), currentRun(type).versions().targetPlatform()); - runner.advance(currentRun(type)); + tester.configServer().nodeRepository().doUpgrade(deployment, Optional.empty(), currentRun(job).versions().targetPlatform()); + runner.advance(currentRun(job)); } /** Lets nodes converge on new application version. */ public void doConverge(JobType type) { - RunId id = currentRun(type).id(); - ZoneId zone = type.zone(tester.controller().system()); + doConverge(new JobId(instanceId, type)); + } + + /** Lets nodes converge on new application version. */ + public void doConverge(ApplicationId instanceId, JobType type) { + doConverge(new JobId(instanceId, type)); + } + + /** Lets nodes converge on new application version. */ + public void doConverge(JobId job) { + RunId id = currentRun(job).id(); + ZoneId zone = job.type().zone(tester.controller().system()); assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installReal)); tester.configServer().convergeServices(instanceId, zone); - runner.advance(currentRun(type)); - if ( ! (currentRun(type).versions().sourceApplication().isPresent() && type.isProduction()) - && type != JobType.stagingTest) { + runner.advance(currentRun(job)); + if ( ! (currentRun(job).versions().sourceApplication().isPresent() && job.type().isProduction()) + && job.type() != JobType.stagingTest) { assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installReal)); setEndpoints(instanceId, zone); } - runner.advance(currentRun(type)); - if (type.environment().isManuallyDeployed()) { + runner.advance(currentRun(job)); + if (job.type().environment().isManuallyDeployed()) { assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installReal)); assertTrue(jobs.run(id).get().hasEnded()); return; @@ -295,24 +325,44 @@ public class InternalDeploymentTester { /** Installs tester and starts tests. */ public void doInstallTester(JobType type) { - RunId id = currentRun(type).id(); - ZoneId zone = type.zone(tester.controller().system()); + doInstallTester(new JobId(instanceId, type)); + } + + /** Installs tester and starts tests. */ + public void doInstallTester(ApplicationId instanceId, JobType type) { + doInstallTester(new JobId(instanceId, type)); + } + + /** Installs tester and starts tests. */ + public void doInstallTester(JobId job) { + RunId id = currentRun(job).id(); + ZoneId zone = job.type().zone(tester.controller().system()); assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installTester)); - tester.configServer().nodeRepository().doUpgrade(new DeploymentId(testerId.id(), zone), Optional.empty(), currentRun(type).versions().targetPlatform()); - runner.advance(currentRun(type)); + tester.configServer().nodeRepository().doUpgrade(new DeploymentId(TesterId.of(job.application()).id(), zone), Optional.empty(), currentRun(job).versions().targetPlatform()); + runner.advance(currentRun(job)); assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installTester)); tester.configServer().convergeServices(testerId.id(), zone); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installTester)); setEndpoints(testerId.id(), zone); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); } /** Completes tests with success. */ public void doTests(JobType type) { - RunId id = currentRun(type).id(); - ZoneId zone = type.zone(tester.controller().system()); + doTests(new JobId(instanceId, type)); + } + + /** Completes tests with success. */ + public void doTests(ApplicationId instanceId, JobType type) { + doTests(new JobId(instanceId, type)); + } + + /** Completes tests with success. */ + public void doTests(JobId job) { + RunId id = currentRun(job).id(); + ZoneId zone = job.type().zone(tester.controller().system()); // All installation is complete and endpoints are ready, so tests may begin. assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installTester)); @@ -320,68 +370,112 @@ public class InternalDeploymentTester { assertEquals(unfinished, jobs.run(id).get().steps().get(Step.endTests)); cloud.set(TesterCloud.Status.SUCCESS); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); assertTrue(jobs.run(id).get().hasEnded()); assertFalse(jobs.run(id).get().hasFailed()); - assertEquals(type.isProduction(), instance().deployments().containsKey(zone)); + assertEquals(job.type().isProduction(), instance().deployments().containsKey(zone)); assertTrue(tester.configServer().nodeRepository().list(zone, testerId.id()).isEmpty()); } /** Removes endpoints from routing layer — always call this. */ public void doTeardown(JobType type) { - ZoneId zone = type.zone(tester.controller().system()); - DeploymentId deployment = new DeploymentId(instanceId, zone); + doTeardown(new JobId(instanceId, type)); + } + + /** Removes endpoints from routing layer — always call this. */ + public void doTeardown(ApplicationId instanceId, JobType type) { + doTeardown(new JobId(instanceId, type)); + } + + /** Removes endpoints from routing layer — always call this. */ + public void doTeardown(JobId job) { + ZoneId zone = job.type().zone(tester.controller().system()); + DeploymentId deployment = new DeploymentId(job.application(), zone); if ( ! instance().deployments().containsKey(zone)) routing.removeEndpoints(deployment); - routing.removeEndpoints(new DeploymentId(testerId.id(), zone)); + routing.removeEndpoints(new DeploymentId(TesterId.of(job.application()).id(), zone)); } /** Pulls the ready job trigger, and then runs the whole of the given job, successfully. */ public void runJob(JobType type) { + runJob(instanceId, type); + } + + /** Pulls the ready job trigger, and then runs the whole of the given job, successfully. */ + public void runJob(ApplicationId instanceId, JobType type) { + runJob(new JobId(instanceId, type)); + } + + /** Pulls the ready job trigger, and then runs the whole of the given job, successfully. */ + public void runJob(JobId job) { tester.readyJobTrigger().run(); - doDeploy(type); - doUpgrade(type); - doConverge(type); - if (type.environment().isManuallyDeployed()) + doDeploy(job); + doUpgrade(job); + doConverge(job); + if (job.type().environment().isManuallyDeployed()) return; - doInstallTester(type); - doTests(type); - doTeardown(type); + doInstallTester(job); + doTests(job); + doTeardown(job); } public void failDeployment(JobType type) { - RunId id = currentRun(type).id(); + failDeployment(new JobId(instanceId, type)); + } + + public void failDeployment(ApplicationId instanceId, JobType type) { + failDeployment(new JobId(instanceId, type)); + } + + public void failDeployment(JobId job) { + RunId id = currentRun(job).id(); tester.readyJobTrigger().run(); tester.configServer().throwOnNextPrepare(new IllegalArgumentException("Exception")); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); assertTrue(jobs.run(id).get().hasFailed()); assertTrue(jobs.run(id).get().hasEnded()); - doTeardown(type); + doTeardown(job); } public void timeOutUpgrade(JobType type) { - RunId id = currentRun(type).id(); + timeOutUpgrade(new JobId(instanceId, type)); + } + + public void timeOutUpgrade(ApplicationId instanceId, JobType type) { + timeOutUpgrade(new JobId(instanceId, type)); + } + + public void timeOutUpgrade(JobId job) { + RunId id = currentRun(job).id(); tester.readyJobTrigger().run(); - doDeploy(type); + doDeploy(job); clock().advance(InternalStepRunner.installationTimeout.plusSeconds(1)); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); assertTrue(jobs.run(id).get().hasFailed()); assertTrue(jobs.run(id).get().hasEnded()); - doTeardown(type); + doTeardown(job); } public void timeOutConvergence(JobType type) { - RunId id = currentRun(type).id(); + timeOutConvergence(new JobId(instanceId, type)); + } + + public void timeOutConvergence(ApplicationId instanceId, JobType type) { + timeOutConvergence(new JobId(instanceId, type)); + } + + public void timeOutConvergence(JobId job) { + RunId id = currentRun(job).id(); tester.readyJobTrigger().run(); - doDeploy(type); - doUpgrade(type); + doDeploy(job); + doUpgrade(job); clock().advance(InternalStepRunner.installationTimeout.plusSeconds(1)); - runner.advance(currentRun(type)); + runner.advance(currentRun(job)); assertTrue(jobs.run(id).get().hasFailed()); assertTrue(jobs.run(id).get().hasEnded()); - doTeardown(type); + doTeardown(job); } public RunId startSystemTestTests() { @@ -403,8 +497,8 @@ public class InternalDeploymentTester { tester.readyJobTrigger().maintain(); if (type.isProduction()) { - runJob(JobType.systemTest); - runJob(JobType.stagingTest); + runJob(new JobId(instanceId, JobType.systemTest)); + runJob(new JobId(instanceId, JobType.stagingTest)); tester.readyJobTrigger().maintain(); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java index 1fd51a453fd..d0c67d67842 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java @@ -1377,4 +1377,25 @@ public class UpgraderTest { assertTrue("Upgrade complete", applications.get().change().isEmpty()); } + @Test + public void testUpgradesPerMinute() { + assertEquals(0, Upgrader.numberOfApplicationsToUpgrade(10, 0, 0)); + + for (long now = 0; now < 60_000; now++) + assertEquals(7, Upgrader.numberOfApplicationsToUpgrade(60_000, now, 7)); + + // Upgrade an app after 8s, 16s, ..., 120s. + assertEquals(3, Upgrader.numberOfApplicationsToUpgrade(30_000, 0, 7.5)); + assertEquals(4, Upgrader.numberOfApplicationsToUpgrade(30_000, 30_000, 7.5)); + assertEquals(4, Upgrader.numberOfApplicationsToUpgrade(30_000, 60_000, 7.5)); + assertEquals(4, Upgrader.numberOfApplicationsToUpgrade(30_000, 90_000, 7.5)); + assertEquals(3, Upgrader.numberOfApplicationsToUpgrade(30_000, 120_000, 7.5)); + + // Run upgrades for 20 minutes. + int upgrades = 0; + for (int i = 0, now = 0; i < 30; i++, now += 40_000) + upgrades += Upgrader.numberOfApplicationsToUpgrade(40_000, now, 8.7); + assertEquals(174, upgrades); + } + } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java index 8a1bbcd09d5..912d285fb52 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java @@ -54,8 +54,8 @@ public class ContainerControllerTester { public ContainerControllerTester(JDisc container, String responseFilePath) { containerTester = new ContainerTester(container, responseFilePath); CuratorDb curatorDb = controller().curator(); - curatorDb.writeUpgradesPerMinute(100); upgrader = new Upgrader(controller(), Duration.ofDays(1), new JobControl(curatorDb), curatorDb); + upgrader.setUpgradesPerMinute(100); // Anything to make it more than one per maintenance interval. } public Controller controller() { return containerTester.controller(); } diff --git a/flags/pom.xml b/flags/pom.xml index c1e9eca20ab..6afa920e261 100644 --- a/flags/pom.xml +++ b/flags/pom.xml @@ -27,6 +27,12 @@ </dependency> <dependency> <groupId>com.yahoo.vespa</groupId> + <artifactId>component</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> <artifactId>defaults</artifactId> <version>${project.version}</version> <scope>provided</scope> diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java index d2228c58c51..4cce7a9bc2a 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java @@ -24,27 +24,40 @@ public class FetchVector { * Note: If this enum is changed, you must also change {@link DimensionHelper}. */ public enum Dimension { - /** - * WARNING: DO NOT USE - * - * <p>ALL flags can be set differently in different zones: This dimension is ONLY useful for the controller - * that needs to handle multiple zones. - * - * <p>Value from ZoneId::value is of the form environment.region. - */ - ZONE_ID, - /** Value from ApplicationId::serializedForm of the form tenant:applicationName:instance. */ APPLICATION_ID, - /** Fully qualified hostname */ - HOSTNAME, - /** Node type from com.yahoo.config.provision.NodeType::name, e.g. tenant, host, confighost, controller, etc. */ NODE_TYPE, /** Cluster type from com.yahoo.config.provision.ClusterSpec.Type::name, e.g. content, container, admin */ - CLUSTER_TYPE + CLUSTER_TYPE, + + /** + * Fully qualified hostname. + * + * <p>NOTE: There is seldom any need to set HOSTNAME, as it is always set implicitly (in {@link Flags}) + * from {@code Defaults.getDefaults().vespaHostname()}. The hostname may e.g. be overridden when + * fetching flag value for a Docker container node. + */ + HOSTNAME, + + /** + * Vespa version from Version::toFullString of the form Major.Minor.Micro. + * + * <p>NOTE: There is seldom any need to set VESPA_VERSION, as it is always set implicitly + * (in {@link Flags}) from {@link com.yahoo.component.Vtag#currentVersion}. The version COULD e.g. + * be overridden when fetching flag value for a Docker container node. + */ + VESPA_VERSION, + + /** + * Zone from ZoneId::value of the form environment.region. + * + * <p>NOTE: There is seldom any need to set ZONE_ID, as all flags are set per zone anyways. The controller + * could PERHAPS use this where it handles multiple zones. + */ + ZONE_ID } private final Map<Dimension, String> map; 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 a27171f29a2..f4b223ead82 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -1,6 +1,7 @@ // Copyright 2019 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.flags; +import com.yahoo.component.Vtag; import com.yahoo.vespa.defaults.Defaults; import com.yahoo.vespa.flags.custom.PreprovisionCapacity; @@ -11,6 +12,7 @@ import java.util.TreeMap; import static com.yahoo.vespa.flags.FetchVector.Dimension.APPLICATION_ID; import static com.yahoo.vespa.flags.FetchVector.Dimension.HOSTNAME; import static com.yahoo.vespa.flags.FetchVector.Dimension.NODE_TYPE; +import static com.yahoo.vespa.flags.FetchVector.Dimension.VESPA_VERSION; /** * Definitions of feature flags. @@ -235,7 +237,8 @@ public class Flags { * from the FlagSource. * @param <T> The boxed type of the flag value, e.g. Boolean for flags guarding features. * @param <U> The type of the unbound flag, e.g. UnboundBooleanFlag. - * @return An unbound flag with {@link FetchVector.Dimension#HOSTNAME HOSTNAME} environment. The ZONE environment + * @return An unbound flag with {@link FetchVector.Dimension#HOSTNAME HOSTNAME} and + * {@link FetchVector.Dimension#VESPA_VERSION VESPA_VERSION} already set. The ZONE environment * is typically implicit. */ private static <T, U extends UnboundFlag<?, ?, ?>> U define(TypedUnboundFlagFactory<T, U> factory, @@ -245,7 +248,11 @@ public class Flags { String modificationEffect, FetchVector.Dimension[] dimensions) { FlagId id = new FlagId(flagId); - FetchVector vector = new FetchVector().with(HOSTNAME, Defaults.getDefaults().vespaHostname()); + FetchVector vector = new FetchVector() + .with(HOSTNAME, Defaults.getDefaults().vespaHostname()) + // Warning: In unit tests and outside official Vespa releases, the currentVersion is e.g. 7.0.0 + // (determined by the current major version). Consider not setting VESPA_VERSION if minor = micro = 0. + .with(VESPA_VERSION, Vtag.currentVersion.toFullString()); U unboundFlag = factory.create(id, defaultValue, vector); FlagDefinition definition = new FlagDefinition(unboundFlag, description, modificationEffect, dimensions); flags.put(id, definition); diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java new file mode 100644 index 00000000000..435ddbb3e19 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/BlacklistCondition.java @@ -0,0 +1,10 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +/** + * @author hakonhall + */ +public class BlacklistCondition extends ListCondition { + public static BlacklistCondition create(CreateParams params) { return new BlacklistCondition(params); } + private BlacklistCondition(CreateParams params) { super(Type.BLACKLIST, params); } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java index a0ad08fb0b3..46961fbd8cc 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/Condition.java @@ -1,62 +1,83 @@ -// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.flags.json; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.json.wire.WireCondition; -import java.util.Arrays; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.function.Predicate; /** * @author hakonhall */ -public class Condition implements Predicate<FetchVector> { - public enum Type { WHITELIST, BLACKLIST } +public interface Condition extends Predicate<FetchVector> { + enum Type { + WHITELIST, + BLACKLIST, + RELATIONAL; - private final Type type; - private final FetchVector.Dimension dimension; - private final List<String> values; + public String toWire() { return name().toLowerCase(); } - public Condition(Type type, FetchVector.Dimension dimension, String... values) { - this(type, dimension, Arrays.asList(values)); - } + public static Type fromWire(String typeString) { + for (Type type : values()) { + if (type.name().equalsIgnoreCase(typeString)) { + return type; + } + } - public Condition(Type type, FetchVector.Dimension dimension, List<String> values) { - this.type = type; - this.dimension = dimension; - this.values = values; + throw new IllegalArgumentException("Unknown type: '" + typeString + "'"); + } } - @Override - public boolean test(FetchVector vector) { - boolean isMember = vector.getValue(dimension).filter(values::contains).isPresent(); + class CreateParams { + private final FetchVector.Dimension dimension; + private List<String> values = List.of(); + private Optional<String> predicate = Optional.empty(); - switch (type) { - case WHITELIST: return isMember; - case BLACKLIST: return !isMember; - default: throw new IllegalArgumentException("Unknown type " + type); + public CreateParams(FetchVector.Dimension dimension) { this.dimension = Objects.requireNonNull(dimension); } + + public CreateParams withValues(String... values) { return withValues(List.of(values)); } + public CreateParams withValues(List<String> values) { + this.values = List.copyOf(values); + return this; } + + public CreateParams withPredicate(String predicate) { + this.predicate = Optional.of(predicate); + return this; + } + + public FetchVector.Dimension dimension() { return dimension; } + public List<String> values() { return values; } + public Optional<String> predicate() { return predicate; } } - public static Condition fromWire(WireCondition wireCondition) { + static Condition fromWire(WireCondition wireCondition) { Objects.requireNonNull(wireCondition.type); - Type type = Type.valueOf(wireCondition.type.toUpperCase()); + Condition.Type type = Condition.Type.fromWire(wireCondition.type); Objects.requireNonNull(wireCondition.dimension); FetchVector.Dimension dimension = DimensionHelper.fromWire(wireCondition.dimension); + var params = new CreateParams(dimension); - List<String> values = wireCondition.values == null ? List.of() : wireCondition.values; + if (wireCondition.values != null) { + params.withValues(wireCondition.values); + } - return new Condition(type, dimension, values); - } + if (wireCondition.predicate != null) { + params.withPredicate(wireCondition.predicate); + } - public WireCondition toWire() { - WireCondition wire = new WireCondition(); - wire.type = type.name().toLowerCase(); - wire.dimension = DimensionHelper.toWire(dimension); - wire.values = values.isEmpty() ? null : values; - return wire; + switch (type) { + case WHITELIST: return WhitelistCondition.create(params); + case BLACKLIST: return BlacklistCondition.create(params); + case RELATIONAL: return RelationalCondition.create(params); + } + + throw new IllegalArgumentException("Unknown type '" + type + "'"); } + + WireCondition toWire(); } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java b/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java index 4fe27e81f2b..c7081ca72ab 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java @@ -18,6 +18,7 @@ public class DimensionHelper { serializedDimensions.put(FetchVector.Dimension.APPLICATION_ID, "application"); serializedDimensions.put(FetchVector.Dimension.NODE_TYPE, "node-type"); serializedDimensions.put(FetchVector.Dimension.CLUSTER_TYPE, "cluster-type"); + serializedDimensions.put(FetchVector.Dimension.VESPA_VERSION, "vespa-version"); if (serializedDimensions.size() != FetchVector.Dimension.values().length) { throw new IllegalStateException(FetchVectorHelper.class.getName() + " is not in sync with " + diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java new file mode 100644 index 00000000000..c2c76529833 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java @@ -0,0 +1,43 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.json.wire.WireCondition; + +import java.util.List; + +/** + * @author hakonhall + */ +public abstract class ListCondition implements Condition { + private final Condition.Type type; + private final FetchVector.Dimension dimension; + private final List<String> values; + private final boolean isWhitelist; + + protected ListCondition(Type type, CreateParams params) { + this.type = type; + this.dimension = params.dimension(); + this.values = List.copyOf(params.values()); + this.isWhitelist = type == Type.WHITELIST; + + if (params.predicate().isPresent()) { + throw new IllegalArgumentException(getClass().getSimpleName() + " does not support the 'predicate' field"); + } + } + + @Override + public boolean test(FetchVector fetchVector) { + boolean listContainsValue = fetchVector.getValue(dimension).map(values::contains).orElse(false); + return isWhitelist == listContainsValue; + } + + @Override + public WireCondition toWire() { + var condition = new WireCondition(); + condition.type = type.toWire(); + condition.dimension = DimensionHelper.toWire(dimension); + condition.values = values.isEmpty() ? null : values; + return condition; + } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java new file mode 100644 index 00000000000..afaf94b26b6 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java @@ -0,0 +1,64 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import com.yahoo.component.Version; +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.json.wire.WireCondition; + +import java.util.List; +import java.util.function.Predicate; + +/** + * @author hakonhall + */ +public class RelationalCondition implements Condition { + private final RelationalPredicate relationalPredicate; + private final Predicate<String> predicate; + private final FetchVector.Dimension dimension; + + public static RelationalCondition create(CreateParams params) { + if (!params.values().isEmpty()) { + throw new IllegalArgumentException(RelationalCondition.class.getSimpleName() + + " does not support the 'values' field"); + } + + String predicate = params.predicate().orElseThrow(() -> + new IllegalArgumentException(RelationalCondition.class.getSimpleName() + + " requires the predicate field in the condition")); + RelationalPredicate relationalPredicate = RelationalPredicate.fromWire(predicate); + + switch (params.dimension()) { + case VESPA_VERSION: + final Version rightVersion = Version.fromString(relationalPredicate.rightOperand()); + Predicate<String> p = (String leftString) -> { + Version leftVersion = Version.fromString(leftString); + return relationalPredicate.operator().evaluate(leftVersion, rightVersion); + }; + return new RelationalCondition(relationalPredicate, p, params.dimension()); + default: + throw new IllegalArgumentException(RelationalCondition.class.getSimpleName() + + " not supported for dimension " + FetchVector.Dimension.VESPA_VERSION.name()); + } + } + + private RelationalCondition(RelationalPredicate relationalPredicate, Predicate<String> predicate, + FetchVector.Dimension dimension) { + this.relationalPredicate = relationalPredicate; + this.predicate = predicate; + this.dimension = dimension; + } + + @Override + public boolean test(FetchVector fetchVector) { + return fetchVector.getValue(dimension).map(predicate::test).orElse(false); + } + + @Override + public WireCondition toWire() { + var condition = new WireCondition(); + condition.type = Condition.Type.RELATIONAL.toWire(); + condition.dimension = DimensionHelper.toWire(dimension); + condition.predicate = relationalPredicate.toWire(); + return condition; + } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java new file mode 100644 index 00000000000..ca7a997f447 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalOperator.java @@ -0,0 +1,39 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import java.util.Objects; +import java.util.function.Function; + +/** + * @author hakonhall + */ +public enum RelationalOperator { + EQUAL ("==", compareToValue -> compareToValue == 0), + NOT_EQUAL ("!=", compareToValue -> compareToValue != 0), + LESS_EQUAL ("<=", compareToValue -> compareToValue <= 0), + LESS ("<" , compareToValue -> compareToValue < 0), + GREATER_EQUAL(">=", compareToValue -> compareToValue >= 0), + GREATER (">" , compareToValue -> compareToValue > 0); + + private String text; + private final Function<Integer, Boolean> compareToValuePredicate; + + RelationalOperator(String text, Function<Integer, Boolean> compareToValuePredicate) { + this.text = text; + this.compareToValuePredicate = compareToValuePredicate; + } + + public String toText() { return text; } + + /** Returns true if 'left op right' is true, with 'op' being the operator represented by this. */ + public <T extends Comparable<T>> boolean evaluate(T left, T right) { + Objects.requireNonNull(left); + Objects.requireNonNull(right); + return evaluate(left.compareTo(right)); + } + + public boolean evaluate(int compareToValue) { + return compareToValuePredicate.apply(compareToValue); + } +} + diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java new file mode 100644 index 00000000000..c5ad195e0d2 --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalPredicate.java @@ -0,0 +1,43 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +import java.util.Comparator; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * @author hakonhall + */ +public class RelationalPredicate { + private final String originalPredicateString; + private final RelationalOperator operator; + private final String rightOperand; + + /** @param predicateString is e.g. "> SUFFIX" or "<=SUFFIX". The first part is {@link RelationalOperator}. */ + public static RelationalPredicate fromWire(String predicateString) { + // Make sure we try to match e.g. "<=" before "<" as the prefix of predicateString. + List<RelationalOperator> operatorsByDecendingLength = Stream.of(RelationalOperator.values()) + .sorted(Comparator.comparing(operator -> - operator.toText().length())) + .collect(Collectors.toList()); + + for (var operator : operatorsByDecendingLength) { + if (predicateString.startsWith(operator.toText())) { + String suffix = predicateString.substring(operator.toText().length()); + return new RelationalPredicate(predicateString, operator, suffix); + } + } + + throw new IllegalArgumentException("Predicate string '" + predicateString + "' does not start with a relation operator"); + } + + private RelationalPredicate(String originalPredicateString, RelationalOperator operator, String rightOperand) { + this.originalPredicateString = originalPredicateString; + this.operator = operator; + this.rightOperand = rightOperand; + } + + public RelationalOperator operator() { return operator; } + public String rightOperand() { return rightOperand; } + public String toWire() { return originalPredicateString; } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java new file mode 100644 index 00000000000..ab266fdaf1d --- /dev/null +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/WhitelistCondition.java @@ -0,0 +1,10 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.flags.json; + +/** + * @author hakonhall + */ +public class WhitelistCondition extends ListCondition { + public static WhitelistCondition create(CreateParams params) { return new WhitelistCondition(params); } + private WhitelistCondition(CreateParams params) { super(Type.WHITELIST, params); } +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java index 2020ce1e49f..1729444fcf2 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/wire/WireCondition.java @@ -16,4 +16,5 @@ public class WireCondition { @JsonProperty("type") public String type; @JsonProperty("dimension") public String dimension; @JsonProperty("values") public List<String> values; + @JsonProperty("predicate") public String predicate; } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java index d19442ae0f0..b41da9f567c 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/ConditionTest.java @@ -4,9 +4,7 @@ package com.yahoo.vespa.flags.json; import com.yahoo.vespa.flags.FetchVector; import org.junit.Test; -import java.util.stream.Collectors; -import java.util.stream.Stream; - +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -17,8 +15,8 @@ public class ConditionTest { @Test public void testWhitelist() { String hostname1 = "host1"; - Condition condition = new Condition(Condition.Type.WHITELIST, FetchVector.Dimension.HOSTNAME, - Stream.of(hostname1).collect(Collectors.toList())); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).withValues(hostname1); + Condition condition = WhitelistCondition.create(params); assertFalse(condition.test(new FetchVector())); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.HOSTNAME, "bar"))); @@ -28,11 +26,38 @@ public class ConditionTest { @Test public void testBlacklist() { String hostname1 = "host1"; - Condition condition = new Condition(Condition.Type.BLACKLIST, FetchVector.Dimension.HOSTNAME, - Stream.of(hostname1).collect(Collectors.toList())); + var params = new Condition.CreateParams(FetchVector.Dimension.HOSTNAME).withValues(hostname1); + Condition condition = BlacklistCondition.create(params); assertTrue(condition.test(new FetchVector())); assertTrue(condition.test(new FetchVector().with(FetchVector.Dimension.APPLICATION_ID, "foo"))); assertTrue(condition.test(new FetchVector().with(FetchVector.Dimension.HOSTNAME, "bar"))); assertFalse(condition.test(new FetchVector().with(FetchVector.Dimension.HOSTNAME, hostname1))); } + + @Test + public void testRelational() { + verifyVespaVersionFor("<", true, false, false); + verifyVespaVersionFor("<=", true, true, false); + verifyVespaVersionFor(">", false, false, true); + verifyVespaVersionFor(">=", false, true, true); + + // Test with empty fetch vector along vespa version dimension (this should never happen as the + // version is always available through Vtag, although Vtag has a dummy version number for e.g. + // locally run unit tests that hasn't set the release Vespa version). + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).withPredicate(">=7.1.2"); + Condition condition = RelationalCondition.create(params); + assertFalse(condition.test(new FetchVector())); + } + + private void verifyVespaVersionFor(String operator, boolean whenLess, boolean whenEqual, boolean whenGreater) { + assertEquals(whenLess, vespaVersionCondition("7.2.4", operator + "7.3.4")); + assertEquals(whenEqual, vespaVersionCondition("7.3.4", operator + "7.3.4")); + assertEquals(whenGreater, vespaVersionCondition("7.4.4", operator + "7.3.4")); + } + + private boolean vespaVersionCondition(String vespaVersion, String predicate) { + var params = new Condition.CreateParams(FetchVector.Dimension.VESPA_VERSION).withPredicate(predicate); + Condition condition = RelationalCondition.create(params); + return condition.test(new FetchVector().with(FetchVector.Dimension.VESPA_VERSION, vespaVersion)); + } } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java index b0e4cd0f682..8326b14fcbf 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java @@ -48,6 +48,11 @@ public class SerializationTest { " \"type\": \"blacklist\",\n" + " \"dimension\": \"hostname\",\n" + " \"values\": [ \"h1\" ]\n" + + " },\n" + + " {\n" + + " \"type\": \"relational\",\n" + + " \"dimension\": \"vespa-version\",\n" + + " \"predicate\": \">=7.3.4\"\n" + " }\n" + " ],\n" + " \"value\": true\n" + @@ -66,7 +71,7 @@ public class SerializationTest { assertThat(wireData.id, equalTo("id2")); // rule assertThat(wireData.rules.size(), equalTo(1)); - assertThat(wireData.rules.get(0).andConditions.size(), equalTo(2)); + assertThat(wireData.rules.get(0).andConditions.size(), equalTo(3)); assertThat(wireData.rules.get(0).value.getNodeType(), equalTo(JsonNodeType.BOOLEAN)); assertThat(wireData.rules.get(0).value.asBoolean(), equalTo(true)); // first condition @@ -79,6 +84,11 @@ public class SerializationTest { assertThat(blacklistCondition.type, equalTo("blacklist")); assertThat(blacklistCondition.dimension, equalTo("hostname")); assertThat(blacklistCondition.values, equalTo(List.of("h1"))); + // third condition + WireCondition relationalCondition = wireData.rules.get(0).andConditions.get(2); + assertThat(relationalCondition.type, equalTo("relational")); + assertThat(relationalCondition.dimension, equalTo("vespa-version")); + assertThat(relationalCondition.predicate, equalTo(">=7.3.4")); // attributes assertThat(wireData.defaultFetchVector, notNullValue()); diff --git a/model-evaluation/abi-spec.json b/model-evaluation/abi-spec.json index a795b0bb447..1857511cbf2 100644 --- a/model-evaluation/abi-spec.json +++ b/model-evaluation/abi-spec.json @@ -8,6 +8,8 @@ "methods": [ "public ai.vespa.models.evaluation.FunctionEvaluator bind(java.lang.String, com.yahoo.tensor.Tensor)", "public ai.vespa.models.evaluation.FunctionEvaluator bind(java.lang.String, double)", + "public ai.vespa.models.evaluation.FunctionEvaluator setMissingValue(com.yahoo.tensor.Tensor)", + "public ai.vespa.models.evaluation.FunctionEvaluator setMissingValue(double)", "public com.yahoo.tensor.Tensor evaluate()", "public com.yahoo.searchlib.rankingexpression.ExpressionFunction function()", "public ai.vespa.models.evaluation.LazyArrayContext context()" @@ -24,6 +26,7 @@ "final" ], "methods": [ + "public void setMissingValue(com.yahoo.tensor.Tensor)", "public void put(java.lang.String, com.yahoo.searchlib.rankingexpression.evaluation.Value)", "public final void put(int, double)", "public void put(int, com.yahoo.searchlib.rankingexpression.evaluation.Value)", diff --git a/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionEvaluator.java b/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionEvaluator.java index caa2db13ff2..994f6dd9b64 100644 --- a/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionEvaluator.java +++ b/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionEvaluator.java @@ -60,10 +60,31 @@ public class FunctionEvaluator { return bind(name, Tensor.Builder.of(TensorType.empty).cell(value).build()); } + /** + * Sets the default value to use for variables which are not bound + * + * @param value the default value + * @return this for chaining + */ + public FunctionEvaluator setMissingValue(Tensor value) { + if (evaluated) + throw new IllegalStateException("Cannot change the missing value in a used evaluator"); + context.setMissingValue(value); + return this; + } + + /** + * Sets the default value to use for variables which are not bound + * + * @param value the default value + * @return this for chaining + */ + public FunctionEvaluator setMissingValue(double value) { + return setMissingValue(Tensor.Builder.of(TensorType.empty).cell(value).build()); + } + public Tensor evaluate() { for (Map.Entry<String, TensorType> argument : function.argumentTypes().entrySet()) { - System.out.println("Checking " + argument.getKey() + " default " + context.defaultValue() + " is assignable to " + argument.getValue() + - "? " + context.defaultValue().type().isAssignableTo(argument.getValue())); if (context.isMissing(argument.getKey())) throw new IllegalStateException("Missing argument '" + argument.getKey() + "': Must be bound to a value of type " + argument.getValue()); diff --git a/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java b/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java index 51daf278a4a..d66315ef457 100644 --- a/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java +++ b/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java @@ -13,6 +13,7 @@ import com.yahoo.searchlib.rankingexpression.evaluation.Value; import com.yahoo.searchlib.rankingexpression.rule.CompositeNode; import com.yahoo.searchlib.rankingexpression.rule.ExpressionNode; import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; +import com.yahoo.tensor.Tensor; import com.yahoo.tensor.TensorType; import java.util.Arrays; @@ -40,10 +41,17 @@ public final class LazyArrayContext extends Context implements ContextIndex { LazyArrayContext(ExpressionFunction function, Map<FunctionReference, ExpressionFunction> referencedFunctions, List<Constant> constants, - Model model, - Value defaultFeatureValue) { + Model model) { this.function = function; - this.indexedBindings = new IndexedBindings(function, referencedFunctions, constants, this, model, defaultFeatureValue); + this.indexedBindings = new IndexedBindings(function, referencedFunctions, constants, this, model); + } + + /** + * Sets the value to use for lookups to existing values which are not set in this context. + * The default value that will be returned is NaN + */ + public void setMissingValue(Tensor value) { + indexedBindings.setMissingValue(value); } /** @@ -90,10 +98,7 @@ public final class LazyArrayContext extends Context implements ContextIndex { @Override public double getDouble(int index) { - double value = get(index).asDouble(); - if (value == Double.NaN) - throw new UnsupportedOperationException("Value at " + index + " has no double representation"); - return value; + return get(index).asDouble(); } @Override @@ -125,7 +130,7 @@ public final class LazyArrayContext extends Context implements ContextIndex { /** Returns the value which should be used when no value is set */ public Value defaultValue() { - return indexedBindings.defaultValue; + return indexedBindings.missingValue; } /** @@ -144,20 +149,21 @@ public final class LazyArrayContext extends Context implements ContextIndex { /** The names which needs to be bound externally when invoking this (i.e not constant or invocation */ private final ImmutableSet<String> arguments; - /** The current values set, pre-converted to doubles */ + /** The current values set */ private final Value[] values; - /** The value to return if not set */ - private final Value defaultValue; + /** The object instance which encodes "no value is set". The actual value of this is never used. */ + private static final Value missing = new DoubleValue(Double.NaN).freeze(); + + /** The value to return for lookups where no value is set (default: NaN) */ + private Value missingValue = new DoubleValue(Double.NaN).freeze(); private IndexedBindings(ImmutableMap<String, Integer> nameToIndex, Value[] values, - ImmutableSet<String> arguments, - Value defaultValue) { + ImmutableSet<String> arguments) { this.nameToIndex = nameToIndex; this.values = values; this.arguments = arguments; - this.defaultValue = defaultValue.freeze(); } /** @@ -168,17 +174,15 @@ public final class LazyArrayContext extends Context implements ContextIndex { Map<FunctionReference, ExpressionFunction> referencedFunctions, List<Constant> constants, LazyArrayContext owner, - Model model, - Value defaultFeatureValue) { + Model model) { // 1. Determine and prepare bind targets Set<String> bindTargets = new LinkedHashSet<>(); Set<String> arguments = new LinkedHashSet<>(); // Arguments: Bind targets which need to be bound before invocation extractBindTargets(function.getBody().getRoot(), referencedFunctions, bindTargets, arguments); this.arguments = ImmutableSet.copyOf(arguments); - this.defaultValue = defaultFeatureValue.freeze(); values = new Value[bindTargets.size()]; - Arrays.fill(values, this.defaultValue); + Arrays.fill(values, missing); int i = 0; ImmutableMap.Builder<String, Integer> nameToIndexBuilder = new ImmutableMap.Builder<>(); @@ -203,6 +207,10 @@ public final class LazyArrayContext extends Context implements ContextIndex { } } + private void setMissingValue(Tensor value) { + missingValue = new TensorValue(value).freeze(); + } + private void extractBindTargets(ExpressionNode node, Map<FunctionReference, ExpressionFunction> functions, Set<String> bindTargets, @@ -241,7 +249,11 @@ public final class LazyArrayContext extends Context implements ContextIndex { return reference.getName().equals("constant") && reference.getArguments().size() == 1; } - Value get(int index) { return values[index]; } + Value get(int index) { + Value value = values[index]; + return value == missing ? missingValue : value; + } + void set(int index, Value value) { values[index] = value; } @@ -254,7 +266,7 @@ public final class LazyArrayContext extends Context implements ContextIndex { Value[] valueCopy = new Value[values.length]; for (int i = 0; i < values.length; i++) valueCopy[i] = values[i] instanceof LazyValue ? ((LazyValue) values[i]).copyFor(context) : values[i]; - return new IndexedBindings(nameToIndex, valueCopy, arguments, defaultValue); + return new IndexedBindings(nameToIndex, valueCopy, arguments); } } diff --git a/model-evaluation/src/main/java/ai/vespa/models/evaluation/Model.java b/model-evaluation/src/main/java/ai/vespa/models/evaluation/Model.java index bc80989f030..90700acc0d3 100644 --- a/model-evaluation/src/main/java/ai/vespa/models/evaluation/Model.java +++ b/model-evaluation/src/main/java/ai/vespa/models/evaluation/Model.java @@ -29,9 +29,6 @@ public class Model { /** The prefix generated by mode-integration/../IntermediateOperation */ private final static String INTERMEDIATE_OPERATION_FUNCTION_PREFIX = "imported_ml_function_"; - /** Default value to return if value is not supplied */ - private final static Value missingValue = DoubleValue.frozen(Double.NaN); - private final String name; /** Free functions */ @@ -66,7 +63,7 @@ public class Model { ImmutableMap.Builder<String, LazyArrayContext> contextBuilder = new ImmutableMap.Builder<>(); for (Map.Entry<FunctionReference, ExpressionFunction> function : functions.entrySet()) { try { - LazyArrayContext context = new LazyArrayContext(function.getValue(), referencedFunctions, constants, this, missingValue); + LazyArrayContext context = new LazyArrayContext(function.getValue(), referencedFunctions, constants, this); contextBuilder.put(function.getValue().getName(), context); if ( ! function.getValue().returnType().isPresent()) { functions.put(function.getKey(), function.getValue().withReturnType(TensorType.empty)); diff --git a/model-evaluation/src/main/java/ai/vespa/models/handler/ModelsEvaluationHandler.java b/model-evaluation/src/main/java/ai/vespa/models/handler/ModelsEvaluationHandler.java index 4ae96bfd62f..5c353fcdf35 100644 --- a/model-evaluation/src/main/java/ai/vespa/models/handler/ModelsEvaluationHandler.java +++ b/model-evaluation/src/main/java/ai/vespa/models/handler/ModelsEvaluationHandler.java @@ -26,6 +26,9 @@ import java.util.concurrent.Executor; public class ModelsEvaluationHandler extends ThreadedHttpRequestHandler { + /** A dash in this key ensures it does not collide with feature names */ + private static final String missingValueKey = "missing-value"; + public static final String API_ROOT = "model-evaluation"; public static final String VERSION_V1 = "v1"; public static final String EVALUATE = "eval"; @@ -70,6 +73,9 @@ public class ModelsEvaluationHandler extends ThreadedHttpRequestHandler { private HttpResponse evaluateModel(HttpRequest request, Model model, String[] function) { FunctionEvaluator evaluator = model.evaluatorOf(function); + + property(request, missingValueKey).ifPresent(missingValue -> evaluator.setMissingValue(Tensor.from(missingValue))); + for (Map.Entry<String, TensorType> argument : evaluator.function().argumentTypes().entrySet()) { property(request, argument.getKey()).ifPresent(value -> evaluator.bind(argument.getKey(), Tensor.from(argument.getValue(), value))); diff --git a/model-evaluation/src/test/java/ai/vespa/models/evaluation/ModelsEvaluatorTest.java b/model-evaluation/src/test/java/ai/vespa/models/evaluation/ModelsEvaluatorTest.java index e8620670dd6..6fcf76d2815 100644 --- a/model-evaluation/src/test/java/ai/vespa/models/evaluation/ModelsEvaluatorTest.java +++ b/model-evaluation/src/test/java/ai/vespa/models/evaluation/ModelsEvaluatorTest.java @@ -7,7 +7,6 @@ import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer; import com.yahoo.path.Path; import com.yahoo.searchlib.rankingexpression.ExpressionFunction; import com.yahoo.searchlib.rankingexpression.RankingExpression; -import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; import com.yahoo.tensor.Tensor; import com.yahoo.tensor.TensorType; import com.yahoo.vespa.config.search.RankProfilesConfig; @@ -19,6 +18,7 @@ import java.util.ArrayList; import java.util.List; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; /** * @author bratseth @@ -36,6 +36,30 @@ public class ModelsEvaluatorTest { assertEquals(32.0, function.evaluate().asDouble(), delta); } + /** Tests a function defined as 4 * (var1 + var2) */ + @Test + public void testSettingMissingValue() { + ModelsEvaluator models = createModels("src/test/resources/config/rankexpression/"); + + { + FunctionEvaluator function = models.evaluatorOf("macros", "secondphase"); + assertTrue(Double.isNaN(function.evaluate().asDouble())); + } + + { + FunctionEvaluator function = models.evaluatorOf("macros", "secondphase"); + function.setMissingValue(5); + assertEquals(40.0, function.evaluate().asDouble(), delta); + } + + { + FunctionEvaluator function = models.evaluatorOf("macros", "secondphase"); + function.setMissingValue(5); + function.bind("match", 3); + assertEquals(32.0, function.evaluate().asDouble(), delta); + } + } + @Test public void testBindingValidation() { List<ExpressionFunction> functions = new ArrayList<>(); diff --git a/model-evaluation/src/test/java/ai/vespa/models/handler/ModelsEvaluationHandlerTest.java b/model-evaluation/src/test/java/ai/vespa/models/handler/ModelsEvaluationHandlerTest.java index 5d30fc93a4c..629c82f410a 100644 --- a/model-evaluation/src/test/java/ai/vespa/models/handler/ModelsEvaluationHandlerTest.java +++ b/model-evaluation/src/test/java/ai/vespa/models/handler/ModelsEvaluationHandlerTest.java @@ -81,6 +81,19 @@ public class ModelsEvaluationHandlerTest { } @Test + public void testXgBoostEvaluationWithMissingValue() { + Map<String, String> properties = new HashMap<>(); + properties.put("missing-value", "-1.0"); + properties.put("f56", "0.2"); + properties.put("f60", "0.3"); + properties.put("f109", "0.4"); + properties.put("non-existing-binding", "-1"); + String url = "http://localhost/model-evaluation/v1/xgboost_2_2/eval"; + String expected = "{\"cells\":[{\"address\":{},\"value\":-7.936679999999999}]}"; + assertResponse(url, properties, 200, expected); + } + + @Test public void testMnistSoftmaxDetails() { String url = "http://localhost:8080/model-evaluation/v1/mnist_softmax"; String expected = "{\"model\":\"mnist_softmax\",\"functions\":[{\"function\":\"default.add\",\"info\":\"http://localhost:8080/model-evaluation/v1/mnist_softmax/default.add\",\"eval\":\"http://localhost:8080/model-evaluation/v1/mnist_softmax/default.add/eval\",\"arguments\":[{\"name\":\"Placeholder\",\"type\":\"tensor(d0[],d1[784])\"}]}]}"; diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java index 59b4a671c55..17dc61978cf 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java @@ -32,6 +32,8 @@ import javax.net.ssl.SSLContext; import java.io.IOException; import java.io.UncheckedIOException; import java.net.URI; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -67,6 +69,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { private final AthenzIdentity configserverIdentity; private final Clock clock; private final ServiceIdentityProvider hostIdentityProvider; + private final FileSystem fileSystem; private final IdentityDocumentClient identityDocumentClient; private final CsrGenerator csrGenerator; private final boolean useInternalZts; @@ -80,16 +83,28 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { String certificateDnsSuffix, ServiceIdentityProvider hostIdentityProvider, boolean useInternalZts) { + this(ztsEndpoint, trustStorePath, configServerInfo, certificateDnsSuffix, hostIdentityProvider, useInternalZts, Clock.systemUTC(), FileSystems.getDefault()); + } + + public AthenzCredentialsMaintainer(URI ztsEndpoint, + Path trustStorePath, + ConfigServerInfo configServerInfo, + String certificateDnsSuffix, + ServiceIdentityProvider hostIdentityProvider, + boolean useInternalZts, + Clock clock, + FileSystem fileSystem) { this.ztsEndpoint = ztsEndpoint; this.trustStorePath = trustStorePath; this.configserverIdentity = configServerInfo.getConfigServerIdentity(); this.csrGenerator = new CsrGenerator(certificateDnsSuffix, configserverIdentity.getFullName()); this.hostIdentityProvider = hostIdentityProvider; + this.fileSystem = fileSystem; this.identityDocumentClient = new DefaultIdentityDocumentClient( configServerInfo.getLoadBalancerEndpoint(), hostIdentityProvider, new AthenzIdentityVerifier(singleton(configserverIdentity))); - this.clock = Clock.systemUTC(); + this.clock = clock; this.useInternalZts = useInternalZts; } @@ -145,6 +160,21 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { lastRefreshAttempt.remove(context.containerName()); } + @Override + public Duration certificateLifetime(NodeAgentContext context) { + Path containerSiaDirectory = fileSystem.getPath(context.pathOnHostFromPathInNode(CONTAINER_SIA_DIRECTORY).toString()); + Path certificateFile = SiaUtils.getCertificateFile(containerSiaDirectory, context.identity()); + try { + X509Certificate certificate = readCertificateFromFile(certificateFile); + Instant now = clock.instant(); + Instant expiry = certificate.getNotAfter().toInstant(); + return Duration.between(now, expiry); + } catch (IOException e) { + context.log(logger, LogLevel.ERROR, "Unable to read certificate at " + certificateFile, e); + return Duration.ZERO; + } + } + private boolean shouldRefreshCredentials(Duration age) { return age.compareTo(REFRESH_PERIOD) >= 0; } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/CredentialsMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/CredentialsMaintainer.java index 58c3585a48f..ca734d73925 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/CredentialsMaintainer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/CredentialsMaintainer.java @@ -3,6 +3,8 @@ package com.yahoo.vespa.hosted.node.admin.maintenance.identity; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import java.time.Duration; + /** * A maintainer that is responsible for providing and refreshing credentials for a container. * @@ -18,4 +20,7 @@ public interface CredentialsMaintainer { /** Remove any existing credentials. This method is called just before container data is archived. */ void clearCredentials(NodeAgentContext context); + + /** Get time until the certificate expires. Invoked each time metrics are collected. */ + Duration certificateLifetime(NodeAgentContext context); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java index 480105f9076..eee0c2cb002 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java @@ -547,4 +547,8 @@ public class NodeAgentImpl implements NodeAgent { } }; } + + protected Optional<CredentialsMaintainer> credentialsMaintainer() { + return credentialsMaintainer; + } } diff --git a/searchlib/src/vespa/searchlib/query/CMakeLists.txt b/searchlib/src/vespa/searchlib/query/CMakeLists.txt index 8ec737657b7..f1f9e64e8d2 100644 --- a/searchlib/src/vespa/searchlib/query/CMakeLists.txt +++ b/searchlib/src/vespa/searchlib/query/CMakeLists.txt @@ -3,7 +3,6 @@ vespa_add_library(searchlib_query OBJECT SOURCES queryterm.cpp querynode.cpp - base.cpp query.cpp query_term_decoder.cpp querynoderesultbase.cpp diff --git a/searchlib/src/vespa/searchlib/query/base.cpp b/searchlib/src/vespa/searchlib/query/base.cpp deleted file mode 100644 index 213fbbb7679..00000000000 --- a/searchlib/src/vespa/searchlib/query/base.cpp +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include "base.h" - -namespace search { - -Object::~Object() { } - -vespalib::string Object::toString() const -{ - return vespalib::string(""); -} - -} diff --git a/searchlib/src/vespa/searchlib/query/base.h b/searchlib/src/vespa/searchlib/query/base.h index c6e0661f82c..da0b815383a 100644 --- a/searchlib/src/vespa/searchlib/query/base.h +++ b/searchlib/src/vespa/searchlib/query/base.h @@ -48,60 +48,5 @@ typedef std::vector<DocumentIdT> DocumentIdList; /// A macro that gives you number of elements in an array. #define NELEMS(a) (sizeof(a)/sizeof(a[0])) -/// A macro used in descendants of Object to instantiate the duplicate method. -#define DUPLICATE(a) a * duplicate() const override; -#define IMPLEMENT_DUPLICATE(a) a * a::duplicate() const { return new a(*this); } - -/** - This is a base class that ensures that all descendants can be duplicated. - This implies also that they have a copy constructor. - It also makes them streamable to an std:ostream. -*/ -class Object -{ - public: - virtual ~Object(); - /// Returns an allocated(new) object that is identical to this one. - virtual Object * duplicate() const = 0; - /// Gives you streamability of the object. Object does nothing. - virtual vespalib::string toString() const; -}; - -/** - This is a template that can hold any objects of any descendants of T. - It does take a copy of the object. Very nice for holding different descendants - and not have to worry about what happens on copy, assignment, destruction. - No references, just simple copy. - It gives you the -> and * operator so you can use it as a pointer to T. - Very convenient. -*/ -template <typename T> -class ObjectContainer -{ - public: - ObjectContainer() : _p(NULL) { } - ObjectContainer(std::unique_ptr<T> &&org) : _p(org.release()) { } - ObjectContainer(const T & org) : _p(static_cast<T*>(org.duplicate())) { } - ObjectContainer(const T * org) : _p(org ? static_cast<T*>(org->duplicate()) : NULL) { } - ObjectContainer(const ObjectContainer & org) : _p(NULL) { *this = org; } - ObjectContainer(ObjectContainer && org) : _p(org._p) { org._p = nullptr; } - ObjectContainer & operator = (const T * org) { cleanUp(); if (org) { _p = static_cast<T*>(org->duplicate()); } return *this; } - ObjectContainer & operator = (const T & org) { cleanUp(); _p = static_cast<T*>(org.duplicate()); return *this; } - ObjectContainer & operator = (const ObjectContainer & org) { if (this != & org) { cleanUp(); if (org._p) { _p = static_cast<T*>(org._p->duplicate());} } return *this; } - ObjectContainer & operator = (ObjectContainer && org) { if (this != & org) { cleanUp(); _p = org._p; org._p = nullptr; } return *this; } - virtual ~ObjectContainer() { cleanUp(); } - bool valid() const { return (_p != NULL); } - T *operator->() { return _p; } - T &operator*() { return *_p; } - const T *operator->() const { return _p; } - const T &operator*() const { return *_p; } - operator T & () const { return *_p; } - operator T * () const { return _p; } - - private: - void cleanUp() { delete _p; _p = NULL; } - T * _p; -}; - } diff --git a/vsm/src/vespa/vsm/searcher/boolfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/boolfieldsearcher.cpp index bf40a679bab..a9f765546a8 100644 --- a/vsm/src/vespa/vsm/searcher/boolfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/boolfieldsearcher.cpp @@ -12,7 +12,11 @@ vespalib::stringref TRUE = "true"; vespalib::stringref FALSE = "false"; } -IMPLEMENT_DUPLICATE(BoolFieldSearcher); +std::unique_ptr<FieldSearcher> +BoolFieldSearcher::duplicate() const +{ + return std::make_unique<BoolFieldSearcher>(*this); +} BoolFieldSearcher::BoolFieldSearcher(FieldIdT fId) : FieldSearcher(fId), diff --git a/vsm/src/vespa/vsm/searcher/boolfieldsearcher.h b/vsm/src/vespa/vsm/searcher/boolfieldsearcher.h index 139645bf0ba..b7f9419465d 100644 --- a/vsm/src/vespa/vsm/searcher/boolfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/boolfieldsearcher.h @@ -8,7 +8,7 @@ namespace vsm { class BoolFieldSearcher : public FieldSearcher { public: - DUPLICATE(BoolFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; BoolFieldSearcher(FieldIdT fId); ~BoolFieldSearcher(); void prepare(search::QueryTermList & qtl, const SharedSearcherBuf & buf) override; diff --git a/vsm/src/vespa/vsm/searcher/fieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/fieldsearcher.cpp index 2ba5fea3153..4bb36d3f968 100644 --- a/vsm/src/vespa/vsm/searcher/fieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/fieldsearcher.cpp @@ -69,7 +69,6 @@ void FieldSearcherBase::prepare(const QueryTermList & qtl) FieldSearcher::FieldSearcher(const FieldIdT & fId, bool defaultPrefix) : FieldSearcherBase(), - Object(), _field(fId), _matchType(defaultPrefix ? PREFIX : REGULAR), _maxFieldLength(0x100000), diff --git a/vsm/src/vespa/vsm/searcher/fieldsearcher.h b/vsm/src/vespa/vsm/searcher/fieldsearcher.h index 2bf976de017..8bfb5a3af31 100644 --- a/vsm/src/vespa/vsm/searcher/fieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/fieldsearcher.h @@ -38,7 +38,7 @@ protected: search::v16qi *_qtlFast; }; -class FieldSearcher : public FieldSearcherBase, public search::Object +class FieldSearcher : public FieldSearcherBase { public: enum MatchType { @@ -50,7 +50,8 @@ public: }; FieldSearcher(const FieldIdT & fId, bool defaultPrefix=false); - ~FieldSearcher(); + ~FieldSearcher() override; + virtual std::unique_ptr<FieldSearcher> duplicate() const = 0; bool search(const StorageDocument & doc); virtual void prepare(search::QueryTermList & qtl, const SharedSearcherBuf & buf); const FieldIdT & field() const { return _field; } @@ -133,7 +134,7 @@ public: static search::byte _wordChar[256]; }; -typedef search::ObjectContainer<FieldSearcher> FieldSearcherContainer; +typedef std::unique_ptr<FieldSearcher> FieldSearcherContainer; typedef std::vector<FieldSearcherContainer> FieldIdTSearcherMapT; class FieldIdTSearcherMap : public FieldIdTSearcherMapT diff --git a/vsm/src/vespa/vsm/searcher/floatfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/floatfieldsearcher.cpp index 0f31c6420be..41a07a03bf2 100644 --- a/vsm/src/vespa/vsm/searcher/floatfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/floatfieldsearcher.cpp @@ -7,8 +7,17 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(FloatFieldSearcher); -IMPLEMENT_DUPLICATE(DoubleFieldSearcher); +std::unique_ptr<FieldSearcher> +FloatFieldSearcher::duplicate() const +{ + return std::make_unique<FloatFieldSearcher>(*this); +} + +std::unique_ptr<FieldSearcher> +DoubleFieldSearcher::duplicate() const +{ + return std::make_unique<DoubleFieldSearcher>(*this); +} template<typename T> FloatFieldSearcherT<T>::FloatFieldSearcherT(FieldIdT fId) : diff --git a/vsm/src/vespa/vsm/searcher/floatfieldsearcher.h b/vsm/src/vespa/vsm/searcher/floatfieldsearcher.h index a589706ff39..5f56ff67b30 100644 --- a/vsm/src/vespa/vsm/searcher/floatfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/floatfieldsearcher.h @@ -38,14 +38,14 @@ typedef FloatFieldSearcherT<double> FloatFieldSearcherTD; class FloatFieldSearcher : public FloatFieldSearcherTF { public: - DUPLICATE(FloatFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; FloatFieldSearcher(FieldIdT fId=0) : FloatFieldSearcherTF(fId) { } }; class DoubleFieldSearcher : public FloatFieldSearcherTD { public: - DUPLICATE(DoubleFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; DoubleFieldSearcher(FieldIdT fId=0) : FloatFieldSearcherTD(fId) { } }; diff --git a/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.cpp index 335f6e81d23..e85cc20d418 100644 --- a/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.cpp @@ -10,7 +10,11 @@ using search::v16qi; namespace vsm { -IMPLEMENT_DUPLICATE(FUTF8StrChrFieldSearcher); +std::unique_ptr<FieldSearcher> +FUTF8StrChrFieldSearcher::duplicate() const +{ + return std::make_unique<FUTF8StrChrFieldSearcher>(*this); +} FUTF8StrChrFieldSearcher::FUTF8StrChrFieldSearcher() : UTF8StrChrFieldSearcher(), diff --git a/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.h b/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.h index 265bca253d9..0539bdc6ea6 100644 --- a/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/futf8strchrfieldsearcher.h @@ -8,7 +8,7 @@ namespace vsm { class FUTF8StrChrFieldSearcher : public UTF8StrChrFieldSearcher { public: - DUPLICATE(FUTF8StrChrFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; FUTF8StrChrFieldSearcher(); FUTF8StrChrFieldSearcher(FieldIdT fId); ~FUTF8StrChrFieldSearcher(); diff --git a/vsm/src/vespa/vsm/searcher/intfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/intfieldsearcher.cpp index 30d9992e2d4..5886a17cf4a 100644 --- a/vsm/src/vespa/vsm/searcher/intfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/intfieldsearcher.cpp @@ -6,7 +6,11 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(IntFieldSearcher); +std::unique_ptr<FieldSearcher> +IntFieldSearcher::duplicate() const +{ + return std::make_unique<IntFieldSearcher>(*this); +} IntFieldSearcher::IntFieldSearcher(FieldIdT fId) : FieldSearcher(fId), diff --git a/vsm/src/vespa/vsm/searcher/intfieldsearcher.h b/vsm/src/vespa/vsm/searcher/intfieldsearcher.h index 3e158b2d93c..ea190bf07cd 100644 --- a/vsm/src/vespa/vsm/searcher/intfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/intfieldsearcher.h @@ -8,7 +8,7 @@ namespace vsm { class IntFieldSearcher : public FieldSearcher { public: - DUPLICATE(IntFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; IntFieldSearcher(FieldIdT fId=0); ~IntFieldSearcher(); void prepare(search::QueryTermList & qtl, const SharedSearcherBuf & buf) override; diff --git a/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.cpp index 26c4021ac40..1de8c40c93a 100644 --- a/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.cpp @@ -7,7 +7,11 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(UTF8ExactStringFieldSearcher); +std::unique_ptr<FieldSearcher> +UTF8ExactStringFieldSearcher::duplicate() const +{ + return std::make_unique<UTF8ExactStringFieldSearcher>(*this); +} size_t UTF8ExactStringFieldSearcher::matchTerms(const FieldRef & f, const size_t mintsz) diff --git a/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.h b/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.h index 9cca519d720..49557b8097d 100644 --- a/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/utf8exactstringfieldsearcher.h @@ -16,7 +16,7 @@ protected: virtual size_t matchTerms(const FieldRef & f, const size_t shortestTerm) override; public: - DUPLICATE(UTF8ExactStringFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; UTF8ExactStringFieldSearcher() : UTF8StringFieldSearcherBase() { } UTF8ExactStringFieldSearcher(FieldIdT fId) : UTF8StringFieldSearcherBase(fId) { } }; diff --git a/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.cpp index b458b73be61..77708a682a2 100644 --- a/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.cpp @@ -9,7 +9,11 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(UTF8FlexibleStringFieldSearcher); +std::unique_ptr<FieldSearcher> +UTF8FlexibleStringFieldSearcher::duplicate() const +{ + return std::make_unique<UTF8FlexibleStringFieldSearcher>(*this); +} size_t UTF8FlexibleStringFieldSearcher::matchTerms(const FieldRef & f, const size_t mintsz) diff --git a/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.h b/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.h index 0e58cae0938..f91bb48a068 100644 --- a/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/utf8flexiblestringfieldsearcher.h @@ -26,7 +26,7 @@ private: virtual size_t matchTerms(const FieldRef & f, const size_t shortestTerm) override; public: - DUPLICATE(UTF8FlexibleStringFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; UTF8FlexibleStringFieldSearcher(); UTF8FlexibleStringFieldSearcher(FieldIdT fId); }; diff --git a/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.cpp index b54ed2c583d..fc78101737e 100644 --- a/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.cpp @@ -7,7 +7,11 @@ using search::byte; namespace vsm { -IMPLEMENT_DUPLICATE(UTF8StrChrFieldSearcher); +std::unique_ptr<FieldSearcher> +UTF8StrChrFieldSearcher::duplicate() const +{ + return std::make_unique<UTF8StrChrFieldSearcher>(*this); +} size_t UTF8StrChrFieldSearcher::matchTerms(const FieldRef & f, const size_t mintsz) diff --git a/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.h b/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.h index b109ede3e03..acfe1256ee5 100644 --- a/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/utf8strchrfieldsearcher.h @@ -12,7 +12,7 @@ namespace vsm { class UTF8StrChrFieldSearcher : public UTF8StringFieldSearcherBase { public: - DUPLICATE(UTF8StrChrFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; UTF8StrChrFieldSearcher() : UTF8StringFieldSearcherBase() { } UTF8StrChrFieldSearcher(FieldIdT fId) : UTF8StringFieldSearcherBase(fId) { } diff --git a/vsm/src/vespa/vsm/searcher/utf8substringsearcher.cpp b/vsm/src/vespa/vsm/searcher/utf8substringsearcher.cpp index 4b8c6e31927..8cda3e27bd5 100644 --- a/vsm/src/vespa/vsm/searcher/utf8substringsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/utf8substringsearcher.cpp @@ -8,7 +8,11 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(UTF8SubStringFieldSearcher); +std::unique_ptr<FieldSearcher> +UTF8SubStringFieldSearcher::duplicate() const +{ + return std::make_unique<UTF8SubStringFieldSearcher>(*this); +} size_t UTF8SubStringFieldSearcher::matchTerms(const FieldRef & f, const size_t mintsz) diff --git a/vsm/src/vespa/vsm/searcher/utf8substringsearcher.h b/vsm/src/vespa/vsm/searcher/utf8substringsearcher.h index e034adeb1b5..9e01a56bdba 100644 --- a/vsm/src/vespa/vsm/searcher/utf8substringsearcher.h +++ b/vsm/src/vespa/vsm/searcher/utf8substringsearcher.h @@ -11,7 +11,7 @@ namespace vsm { class UTF8SubStringFieldSearcher : public UTF8StringFieldSearcherBase { public: - DUPLICATE(UTF8SubStringFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; UTF8SubStringFieldSearcher() : UTF8StringFieldSearcherBase() { } UTF8SubStringFieldSearcher(FieldIdT fId) : UTF8StringFieldSearcherBase(fId) { } protected: diff --git a/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.cpp b/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.cpp index eee88b34ea6..c19259273d0 100644 --- a/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.cpp +++ b/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.cpp @@ -8,7 +8,11 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(UTF8SubstringSnippetModifier); +std::unique_ptr<FieldSearcher> +UTF8SubstringSnippetModifier::duplicate() const +{ + return std::make_unique<UTF8SubstringSnippetModifier>(*this); +} size_t UTF8SubstringSnippetModifier::matchTerms(const FieldRef & f, const size_t mintsz) diff --git a/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.h b/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.h index 3d266174f3d..fce0fdc2175 100644 --- a/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.h +++ b/vsm/src/vespa/vsm/searcher/utf8substringsnippetmodifier.h @@ -49,7 +49,7 @@ private: public: typedef std::shared_ptr<UTF8SubstringSnippetModifier> SP; - DUPLICATE(UTF8SubstringSnippetModifier); + std::unique_ptr<FieldSearcher> duplicate() const override; UTF8SubstringSnippetModifier(); UTF8SubstringSnippetModifier(FieldIdT fId); diff --git a/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.cpp b/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.cpp index 13074937185..1e9ec246743 100644 --- a/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.cpp +++ b/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.cpp @@ -7,7 +7,11 @@ using search::QueryTermList; namespace vsm { -IMPLEMENT_DUPLICATE(UTF8SuffixStringFieldSearcher); +std::unique_ptr<FieldSearcher> +UTF8SuffixStringFieldSearcher::duplicate() const +{ + return std::make_unique<UTF8SuffixStringFieldSearcher>(*this); +} size_t UTF8SuffixStringFieldSearcher::matchTerms(const FieldRef & f, const size_t mintsz) diff --git a/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.h b/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.h index 8bb32ab3c39..cc55ee7bb43 100644 --- a/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.h +++ b/vsm/src/vespa/vsm/searcher/utf8suffixstringfieldsearcher.h @@ -16,7 +16,7 @@ protected: virtual size_t matchTerms(const FieldRef & f, const size_t shortestTerm) override; public: - DUPLICATE(UTF8SuffixStringFieldSearcher); + std::unique_ptr<FieldSearcher> duplicate() const override; UTF8SuffixStringFieldSearcher() : UTF8StringFieldSearcherBase() { } UTF8SuffixStringFieldSearcher(FieldIdT fId) : UTF8StringFieldSearcherBase(fId) { } }; diff --git a/vsm/src/vespa/vsm/vsm/fieldsearchspec.cpp b/vsm/src/vespa/vsm/vsm/fieldsearchspec.cpp index 8d4cf72b824..8bb047beb24 100644 --- a/vsm/src/vespa/vsm/vsm/fieldsearchspec.cpp +++ b/vsm/src/vespa/vsm/vsm/fieldsearchspec.cpp @@ -52,6 +52,9 @@ FieldSearchSpec::FieldSearchSpec() : } FieldSearchSpec::~FieldSearchSpec() = default; +FieldSearchSpec& +FieldSearchSpec::operator=(FieldSearchSpec&& rhs) = default; + FieldSearchSpec::FieldSearchSpec(const FieldIdT & fid, const vespalib::string & fname, VsmfieldsConfig::Fieldspec::Searchmethod searchDef, const vespalib::string & arg1, size_t maxLength_) : @@ -72,36 +75,36 @@ FieldSearchSpec::FieldSearchSpec(const FieldIdT & fid, const vespalib::string & case VsmfieldsConfig::Fieldspec::Searchmethod::SSE2UTF8: case VsmfieldsConfig::Fieldspec::Searchmethod::UTF8: if (arg1 == "substring") { - _searcher = UTF8SubStringFieldSearcher(fid); + _searcher = std::make_unique<UTF8SubStringFieldSearcher>(fid); } else if (arg1 == "suffix") { - _searcher = UTF8SuffixStringFieldSearcher(fid); + _searcher = std::make_unique<UTF8SuffixStringFieldSearcher>(fid); } else if (arg1 == "exact") { - _searcher = UTF8ExactStringFieldSearcher(fid); + _searcher = std::make_unique<UTF8ExactStringFieldSearcher>(fid); } else if (arg1 == "word") { - _searcher = UTF8ExactStringFieldSearcher(fid); + _searcher = std::make_unique<UTF8ExactStringFieldSearcher>(fid); } else if (searchDef == VsmfieldsConfig::Fieldspec::Searchmethod::UTF8) { - _searcher = UTF8StrChrFieldSearcher(fid); + _searcher = std::make_unique<UTF8StrChrFieldSearcher>(fid); } else { - _searcher = FUTF8StrChrFieldSearcher(fid); + _searcher = std::make_unique<FUTF8StrChrFieldSearcher>(fid); } break; case VsmfieldsConfig::Fieldspec::Searchmethod::BOOL: - _searcher = BoolFieldSearcher(fid); + _searcher = std::make_unique<BoolFieldSearcher>(fid); break; case VsmfieldsConfig::Fieldspec::Searchmethod::INT8: case VsmfieldsConfig::Fieldspec::Searchmethod::INT16: case VsmfieldsConfig::Fieldspec::Searchmethod::INT32: case VsmfieldsConfig::Fieldspec::Searchmethod::INT64: - _searcher = IntFieldSearcher(fid); + _searcher = std::make_unique<IntFieldSearcher>(fid); break; case VsmfieldsConfig::Fieldspec::Searchmethod::FLOAT: - _searcher = FloatFieldSearcher(fid); + _searcher = std::make_unique<FloatFieldSearcher>(fid); break; case VsmfieldsConfig::Fieldspec::Searchmethod::DOUBLE: - _searcher = DoubleFieldSearcher(fid); + _searcher = std::make_unique<DoubleFieldSearcher>(fid); break; } - if (_searcher.valid()) { + if (_searcher) { setMatchType(_searcher, arg1); _searcher->maxFieldLength(maxLength()); } @@ -123,7 +126,7 @@ FieldSearchSpec::reconfig(const search::QueryTerm & term) (term.isExactstring() && _arg1 != "exact") || (term.isPrefix() && _arg1 == "suffix")) { - _searcher = UTF8FlexibleStringFieldSearcher(id()); + _searcher = std::make_unique<UTF8FlexibleStringFieldSearcher>(id()); // preserve the basic match property of the searcher setMatchType(_searcher, _arg1); LOG(debug, "Reconfigured to use UTF8FlexibleStringFieldSearcher (%s) for field '%s' with id '%d'", @@ -139,7 +142,7 @@ FieldSearchSpec::reconfig(const search::QueryTerm & term) vespalib::asciistream & operator <<(vespalib::asciistream & os, const FieldSearchSpec & f) { os << f._id << ' ' << f._name << ' '; - if ( ! f._searcher.valid()) { + if ( ! f._searcher) { os << " No searcher defined.\n"; } return os; @@ -251,7 +254,7 @@ bool FieldSearchSpecMap::buildFromConfig(const VsmfieldsHandle & conf) LOG(spam, "Parsing %s", cfs.name.c_str()); FieldIdT fieldId = specMap().size(); FieldSearchSpec fss(fieldId, cfs.name, cfs.searchmethod, cfs.arg1.c_str(), cfs.maxlength); - _specMap[fieldId] = fss; + _specMap[fieldId] = std::move(fss); _nameIdMap.add(cfs.name, fieldId); LOG(spam, "M in %d = %s", fieldId, cfs.name.c_str()); } @@ -298,7 +301,7 @@ void FieldSearchSpecMap::buildSearcherMap(const StringFieldIdTMapT & fieldsInQue for (const auto & entry : fieldsInQuery) { FieldIdT fId = entry.second; const FieldSearchSpec & spec = specMap().find(fId)->second; - fieldSearcherMap.push_back(spec.searcher()); + fieldSearcherMap.emplace_back(spec.searcher().duplicate()); } std::sort(fieldSearcherMap.begin(), fieldSearcherMap.end(), lesserField); } diff --git a/vsm/src/vespa/vsm/vsm/fieldsearchspec.h b/vsm/src/vespa/vsm/vsm/fieldsearchspec.h index 445334bd182..d3feb8621b5 100644 --- a/vsm/src/vespa/vsm/vsm/fieldsearchspec.h +++ b/vsm/src/vespa/vsm/vsm/fieldsearchspec.h @@ -14,10 +14,11 @@ public: VsmfieldsConfig::Fieldspec::Searchmethod searchMethod, const vespalib::string & arg1, size_t maxLength); ~FieldSearchSpec(); + FieldSearchSpec& operator=(FieldSearchSpec&& rhs); const FieldSearcher & searcher() const { return *_searcher; } const vespalib::string & name() const { return _name; } FieldIdT id() const { return _id; } - bool valid() const { return _searcher.valid(); } + bool valid() const { return static_cast<bool>(_searcher); } size_t maxLength() const { return _maxLength; } /** |