diff options
27 files changed, 359 insertions, 337 deletions
diff --git a/client/go/cmd/prod_test.go b/client/go/cmd/prod_test.go index 3c1799701a3..eff06cf860e 100644 --- a/client/go/cmd/prod_test.go +++ b/client/go/cmd/prod_test.go @@ -217,6 +217,30 @@ func TestProdSubmitWithJava(t *testing.T) { assert.Contains(t, stdout.String(), "See https://console.vespa-cloud.com/tenant/t1/application/a1/prod/deployment for deployment progress") } +func TestProdSubmitInvalidZip(t *testing.T) { + pkgDir := filepath.Join(t.TempDir(), "app") + createApplication(t, pkgDir, true) + + httpClient := &mock.HTTPClient{} + httpClient.NextResponseString(200, `ok`) + cli, _, stderr := newTestCLI(t, "CI=true") + cli.httpClient = httpClient + assert.Nil(t, cli.Run("config", "set", "application", "t1.a1.i1")) + assert.Nil(t, cli.Run("config", "set", "target", "cloud")) + assert.Nil(t, cli.Run("auth", "api-key")) + assert.Nil(t, cli.Run("auth", "cert", pkgDir)) + + // Copy an invalid application package containing relative file names + testAppDir := filepath.Join("testdata", "applications", "withInvalidEntries", "target") + zipFile := filepath.Join(testAppDir, "application.zip") + copyFile(t, filepath.Join(pkgDir, "target", "application.zip"), zipFile) + testZipFile := filepath.Join(testAppDir, "application-test.zip") + copyFile(t, filepath.Join(pkgDir, "target", "application-test.zip"), testZipFile) + + assert.NotNil(t, cli.Run("prod", "submit", pkgDir)) + assert.Equal(t, "Error: found invalid path inside zip: ../../../../../../../tmp/foo\n", stderr.String()) +} + func copyFile(t *testing.T, dstFilename, srcFilename string) { dst, err := os.Create(dstFilename) if err != nil { diff --git a/client/go/cmd/testdata/applications/withInvalidEntries/target/application-test.zip b/client/go/cmd/testdata/applications/withInvalidEntries/target/application-test.zip Binary files differnew file mode 100644 index 00000000000..3e4e8c23f9b --- /dev/null +++ b/client/go/cmd/testdata/applications/withInvalidEntries/target/application-test.zip diff --git a/client/go/cmd/testdata/applications/withInvalidEntries/target/application.zip b/client/go/cmd/testdata/applications/withInvalidEntries/target/application.zip Binary files differnew file mode 100644 index 00000000000..3e4e8c23f9b --- /dev/null +++ b/client/go/cmd/testdata/applications/withInvalidEntries/target/application.zip diff --git a/client/go/vespa/application.go b/client/go/vespa/application.go index 06c643c2b3f..2f19c00b3f2 100644 --- a/client/go/vespa/application.go +++ b/client/go/vespa/application.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/vespa-engine/vespa/client/go/util" ) @@ -165,6 +166,9 @@ func (ap *ApplicationPackage) Unzip(test bool) (string, error) { } defer f.Close() for _, f := range f.File { + if !validPath(f.Name) { + return "", fmt.Errorf("found invalid path inside zip: %s", f.Name) + } dst := filepath.Join(tmp, f.Name) if f.FileInfo().IsDir() { if err := os.Mkdir(dst, f.FileInfo().Mode()); err != nil { @@ -173,14 +177,26 @@ func (ap *ApplicationPackage) Unzip(test bool) (string, error) { continue } if err := copyFile(f, dst); err != nil { - return "", fmt.Errorf("copyFile: %w", err) + return "", fmt.Errorf("copying %s to %s failed: %w", f.Name, dst, err) } - } cleanTemp = false return tmp, nil } +func validPath(path string) bool { + path = strings.TrimSuffix(path, "/") + if filepath.Clean(path) != path { + return false + } + for _, part := range strings.Split(path, "/") { + if part == ".." { + return false + } + } + return true +} + func copyFile(src *zip.File, dst string) error { from, err := src.Open() if err != nil { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java index 1548f6ea728..6777e2fb741 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java @@ -409,17 +409,11 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon int pos = nameAndValue.indexOf('='); environmentVariables.put(nameAndValue.substring(0, pos), nameAndValue.substring(pos+1)); } - public void addEnvironmentVariable(String name, Object value) { environmentVariables.put(name, value); } - @Override - public Map<String, Object> getEnvVars() { - return Map.copyOf(environmentVariables); - } - - public String getEnvStringForTesting() { + public String getEnv() { return environmentVariables.entrySet().stream().map(e -> e.getKey() + '=' + toEnvValue(e.getValue())).collect(Collectors.joining(" ")); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java b/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java index d3fad2d94d1..217f6cff778 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java @@ -99,9 +99,6 @@ public class ConfigSentinel extends AbstractService implements SentinelConfig.Pr serviceBuilder.name(s.getServiceName()); serviceBuilder.id(s.getConfigId()); serviceBuilder.affinity(getServiceAffinity(s)); - for (var entry : s.getEnvVars().entrySet()) { - serviceBuilder.environ(b -> b.varname(entry.getKey()).varvalue(entry.getValue().toString())); - } setPreShutdownCommand(serviceBuilder, s); return serviceBuilder; } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/Service.java b/config-model/src/main/java/com/yahoo/vespa/model/Service.java index 87fd8078c3f..3849a57db6f 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/Service.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/Service.java @@ -4,7 +4,6 @@ package com.yahoo.vespa.model; import com.yahoo.config.model.api.ServiceInfo; import java.util.HashMap; -import java.util.Map; import java.util.Optional; /** @@ -22,9 +21,6 @@ public interface Service extends ConfigProducer, NetworkPortRequestor { */ String getStartupCommand(); - // environment variables specific for this service: - Map<String, Object> getEnvVars(); - /** * Services that wish that a command should be run before shutdown * should return the command here. The command will be executed diff --git a/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java b/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java index 40e57e2f962..3d4e0bd22d4 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java @@ -109,10 +109,10 @@ public class SearchNode extends AbstractService implements SearchNode node = new SearchNode(parent, name, distributionKey, nodeSpec, clusterName, serviceLayerService, flushOnShutdown, tuning, resourceLimits, isHostedVespa, fractionOfMemoryReserved); if (featureFlags.loadCodeAsHugePages()) { - node.addEnvironmentVariable("VESPA_LOAD_CODE_AS_HUGEPAGES", true); + node.addEnvironmentVariable("VESPA_LOAD_CODE_AS_HUGEPAGES", "true"); } - if (featureFlags.sharedStringRepoNoReclaim()) { - node.addEnvironmentVariable("VESPA_SHARED_STRING_REPO_NO_RECLAIM", true); + if ( featureFlags.sharedStringRepoNoReclaim()) { + node.addEnvironmentVariable("VESPA_SHARED_STRING_REPO_NO_RECLAIM", "true"); } return node; } @@ -240,7 +240,7 @@ public class SearchNode extends AbstractService implements @Override public String getStartupCommand() { - String startup = "exec $ROOT/sbin/vespa-proton --identity " + getConfigId(); + String startup = getEnv() + " exec $ROOT/sbin/vespa-proton " + "--identity " + getConfigId(); if (serviceLayerService != null) { startup = startup + " --serviceidentity " + serviceLayerService.getConfigId(); } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/ContentBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/ContentBuilderTest.java index 6ac4dbd17e3..0848b8becb9 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/ContentBuilderTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/ContentBuilderTest.java @@ -307,8 +307,8 @@ public class ContentBuilderTest extends DomBuilderTest { assertEquals(200000, b.getRootGroup().getMmapNoCoreLimit().get().longValue()); assertEquals(2, s.getSearchNodes().size()); - assertTrue(s.getSearchNodes().get(0).getEnvStringForTesting().contains("VESPA_MMAP_NOCORE_LIMIT=200000")); - assertTrue(s.getSearchNodes().get(1).getEnvStringForTesting().contains("VESPA_MMAP_NOCORE_LIMIT=200000")); + assertTrue(s.getSearchNodes().get(0).getEnv().contains("VESPA_MMAP_NOCORE_LIMIT=200000")); + assertTrue(s.getSearchNodes().get(1).getEnv().contains("VESPA_MMAP_NOCORE_LIMIT=200000")); } @Test @@ -334,12 +334,12 @@ public class ContentBuilderTest extends DomBuilderTest { AbstractService node = s.getSearchNodes().get(0); node.addEnvironmentVariable("MY_ENV_1", 7); node.addEnvironmentVariable("MY_ENV_2", "7 8"); - assertTrue(node.getEnvStringForTesting().contains("MY_ENV_1=7")); - assertTrue(node.getEnvStringForTesting().contains("MY_ENV_2=\"7 8\"")); + assertTrue(node.getEnv().contains("MY_ENV_1=7")); + assertTrue(node.getEnv().contains("MY_ENV_2=\"7 8\"")); node.addEnvironmentVariable("MY_PARSED_ENV_1=7"); node.addEnvironmentVariable("MY_PARSED_ENV_2=7 8"); - assertTrue(node.getEnvStringForTesting().contains("MY_PARSED_ENV_1=\"7\"")); - assertTrue(node.getEnvStringForTesting().contains("MY_PARSED_ENV_2=\"7 8\"")); + assertTrue(node.getEnv().contains("MY_PARSED_ENV_1=\"7\"")); + assertTrue(node.getEnv().contains("MY_PARSED_ENV_2=\"7 8\"")); } @Test @@ -363,8 +363,8 @@ public class ContentBuilderTest extends DomBuilderTest { assertEquals(1, s.getSearchNodes().size()); AbstractService node = s.getSearchNodes().get(0); - assertTrue(node.getEnvStringForTesting().contains("MY_1_ENV=\"xyz abc\"")); - assertTrue(node.getEnvStringForTesting().contains("MY_2_ENV=\"2\"")); + assertTrue(node.getEnv().contains("MY_1_ENV=\"xyz abc\"")); + assertTrue(node.getEnv().contains("MY_2_ENV=\"2\"")); } @Test @@ -390,8 +390,8 @@ public class ContentBuilderTest extends DomBuilderTest { assertTrue(b.getRootGroup().getCoreOnOOM().get()); assertEquals(2, s.getSearchNodes().size()); - assertFalse(s.getSearchNodes().get(0).getEnvStringForTesting().contains("VESPA_SILENCE_CORE_ON_OOM=true")); - assertFalse(s.getSearchNodes().get(1).getEnvStringForTesting().contains("VESPA_SILENCE_CORE_ON_OOM=true")); + assertFalse(s.getSearchNodes().get(0).getEnv().contains("VESPA_SILENCE_CORE_ON_OOM=true")); + assertFalse(s.getSearchNodes().get(1).getEnv().contains("VESPA_SILENCE_CORE_ON_OOM=true")); } @Test @@ -414,8 +414,8 @@ public class ContentBuilderTest extends DomBuilderTest { assertFalse(b.getRootGroup().getCoreOnOOM().isPresent()); assertEquals(2, s.getSearchNodes().size()); - assertTrue(s.getSearchNodes().get(0).getEnvStringForTesting().contains("VESPA_SILENCE_CORE_ON_OOM=true")); - assertTrue(s.getSearchNodes().get(1).getEnvStringForTesting().contains("VESPA_SILENCE_CORE_ON_OOM=true")); + assertTrue(s.getSearchNodes().get(0).getEnv().contains("VESPA_SILENCE_CORE_ON_OOM=true")); + assertTrue(s.getSearchNodes().get(1).getEnv().contains("VESPA_SILENCE_CORE_ON_OOM=true")); } @Test @@ -438,8 +438,8 @@ public class ContentBuilderTest extends DomBuilderTest { assertFalse(b.getRootGroup().getMmapNoCoreLimit().isPresent()); assertEquals(2, s.getSearchNodes().size()); - assertTrue(s.getSearchNodes().get(0).getEnvStringForTesting().contains("VESPA_MMAP_NOCORE_LIMIT=200000")); - assertFalse(s.getSearchNodes().get(1).getEnvStringForTesting().contains("VESPA_MMAP_NOCORE_LIMIT=")); + assertTrue(s.getSearchNodes().get(0).getEnv().contains("VESPA_MMAP_NOCORE_LIMIT=200000")); + assertFalse(s.getSearchNodes().get(1).getEnv().contains("VESPA_MMAP_NOCORE_LIMIT=")); } @Test @@ -462,8 +462,8 @@ public class ContentBuilderTest extends DomBuilderTest { assertFalse(b.getRootGroup().getCoreOnOOM().isPresent()); assertEquals(2, s.getSearchNodes().size()); - assertFalse(s.getSearchNodes().get(0).getEnvStringForTesting().contains("VESPA_SILENCE_CORE_ON_OOM=true")); - assertTrue(s.getSearchNodes().get(1).getEnvStringForTesting().contains("VESPA_SILENCE_CORE_ON_OOM=true")); + assertFalse(s.getSearchNodes().get(0).getEnv().contains("VESPA_SILENCE_CORE_ON_OOM=true")); + assertTrue(s.getSearchNodes().get(1).getEnv().contains("VESPA_SILENCE_CORE_ON_OOM=true")); } @Test @@ -496,7 +496,7 @@ public class ContentBuilderTest extends DomBuilderTest { assertEquals(4, s.getSearchNodes().size()); for (SearchNode n : s.getSearchNodes()) { - assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_NO_VESPAMALLOC=\"proton\" VESPA_USE_VESPAMALLOC=\"storaged\" VESPA_USE_VESPAMALLOC_D=\"distributord\" VESPA_USE_VESPAMALLOC_DST=\"all\"", n.getEnvStringForTesting()); + assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_NO_VESPAMALLOC=\"proton\" VESPA_USE_VESPAMALLOC=\"storaged\" VESPA_USE_VESPAMALLOC_D=\"distributord\" VESPA_USE_VESPAMALLOC_DST=\"all\"", n.getEnv()); } } @@ -525,10 +525,10 @@ public class ContentBuilderTest extends DomBuilderTest { assertFalse(b.getRootGroup().getVespaMallocDebugStackTrace().isPresent()); assertEquals(4, s.getSearchNodes().size()); - assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_NO_VESPAMALLOC=\"proton\"", s.getSearchNodes().get(0).getEnvStringForTesting()); - assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_VESPAMALLOC_D=\"distributord\"", s.getSearchNodes().get(1).getEnvStringForTesting()); - assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_VESPAMALLOC_DST=\"all\"", s.getSearchNodes().get(2).getEnvStringForTesting()); - assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_VESPAMALLOC=\"storaged\"", s.getSearchNodes().get(3).getEnvStringForTesting()); + assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_NO_VESPAMALLOC=\"proton\"", s.getSearchNodes().get(0).getEnv()); + assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_VESPAMALLOC_D=\"distributord\"", s.getSearchNodes().get(1).getEnv()); + assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_VESPAMALLOC_DST=\"all\"", s.getSearchNodes().get(2).getEnv()); + assertEquals("OMP_NUM_THREADS=1 VESPA_SILENCE_CORE_ON_OOM=true VESPA_USE_VESPAMALLOC=\"storaged\"", s.getSearchNodes().get(3).getEnv()); } @Test diff --git a/config-model/src/test/java/com/yahoo/vespa/model/search/test/SearchNodeTest.java b/config-model/src/test/java/com/yahoo/vespa/model/search/test/SearchNodeTest.java index f60441bac03..7ed27018d5f 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/search/test/SearchNodeTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/search/test/SearchNodeTest.java @@ -95,7 +95,7 @@ public class SearchNodeTest { SearchNode node = createSearchNode(root, "mynode2", 4, new NodeSpec(7, 5), true, false, new TestProperties().loadCodeAsHugePages(hugePages)); node.setHostResource(new HostResource(new Host(node, "mynbode2"))); node.initService(root.getDeployState()); - assertEquals(hugePages, node.getEnvVars().get("VESPA_LOAD_CODE_AS_HUGEPAGES") != null); + assertEquals(hugePages, node.getStartupCommand().contains("VESPA_LOAD_CODE_AS_HUGEPAGES=")); } @Test @@ -109,7 +109,7 @@ public class SearchNodeTest { SearchNode node = createSearchNode(root, "mynode2", 4, new NodeSpec(7, 5), true, false, new TestProperties().sharedStringRepoNoReclaim(sharedStringRepoNoReclaim)); node.setHostResource(new HostResource(new Host(node, "mynbode2"))); node.initService(root.getDeployState()); - assertEquals(sharedStringRepoNoReclaim, node.getEnvVars().get("VESPA_SHARED_STRING_REPO_NO_RECLAIM") != null); + assertEquals(sharedStringRepoNoReclaim, node.getStartupCommand().contains("VESPA_SHARED_STRING_REPO_NO_RECLAIM=")); } @Test diff --git a/configd/src/apps/sentinel/service.cpp b/configd/src/apps/sentinel/service.cpp index e9225325e27..16ca132c29d 100644 --- a/configd/src/apps/sentinel/service.cpp +++ b/configd/src/apps/sentinel/service.cpp @@ -326,10 +326,6 @@ Service::runChild() fcntl(n, F_SETFD, FD_CLOEXEC); } - for (const auto &envvar : _config->environ) { - setenv(envvar.varname.c_str(), envvar.varvalue.c_str(), 1); - } - // Set up environment setenv("VESPA_SERVICE_NAME", _config->name.c_str(), 1); setenv("VESPA_CONFIG_ID", _config->id.c_str(), 1); diff --git a/configdefinitions/src/vespa/sentinel.def b/configdefinitions/src/vespa/sentinel.def index f28c43c77d4..7bd4a000055 100644 --- a/configdefinitions/src/vespa/sentinel.def +++ b/configdefinitions/src/vespa/sentinel.def @@ -39,10 +39,6 @@ ignoreRequestedStackSizes bool default=false restart ## stdout and stderr connected to sentinel via pipe on startup. service[].command string -## Extra environment variables that will be exposed for this service -service[].environ[].varname string -service[].environ[].varvalue string - ## The command to run before stopping service. The same properties as for ## startup command holds. service[].preShutdownCommand string default="" diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/CoverageAggregator.java b/container-search/src/main/java/com/yahoo/search/dispatch/CoverageAggregator.java index 35d0a1f3231..4e19c8a2e22 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/CoverageAggregator.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/CoverageAggregator.java @@ -69,32 +69,34 @@ public class CoverageAggregator { coverage.setDegradedReason(degradedReason); return coverage; } - public CoverageAggregator adjustedDegradedCoverage(int searchableCopies, TimeoutHandler timeoutHandler) { + public CoverageAggregator adjustedDegradedCoverage(int redundancy, TimeoutHandler timeoutHandler) { int askedAndFailed = askedNodes + failedNodes; if (askedAndFailed == answeredNodesParticipated) { return this; } int notAnswered = askedAndFailed - answeredNodesParticipated; - if ((timeoutHandler.reason() == DEGRADED_BY_ADAPTIVE_TIMEOUT) && answeredNodesParticipated > 0) { + if (timeoutHandler.reason() == DEGRADED_BY_ADAPTIVE_TIMEOUT) { CoverageAggregator clone = new CoverageAggregator(this); - clone.answeredActiveDocs += (notAnswered * answeredActiveDocs / answeredNodesParticipated); - clone.answeredTargetActiveDocs += (notAnswered * answeredTargetActiveDocs / answeredNodesParticipated); - return clone; + return clone.adjustActiveDocs(notAnswered); } else { if (askedAndFailed > answeredNodesParticipated) { - if (answeredNodesParticipated > 0) { - CoverageAggregator clone = new CoverageAggregator(this); - int missingNodes = notAnswered - (searchableCopies - 1); - if (missingNodes > 0) { - clone.answeredActiveDocs += (missingNodes * answeredActiveDocs / answeredNodesParticipated); - clone.answeredTargetActiveDocs += (missingNodes * answeredTargetActiveDocs / answeredNodesParticipated); - } - clone.timedOut = true; - return clone; + CoverageAggregator clone = new CoverageAggregator(this); + int missingNodes = notAnswered - (redundancy - 1); + if (missingNodes > 0) { + clone.adjustActiveDocs(missingNodes); } + clone.timedOut = true; + return clone; } } return this; } + private CoverageAggregator adjustActiveDocs(int numMissingNodes) { + if (answeredNodesParticipated > 0) { + answeredActiveDocs += (numMissingNodes * answeredActiveDocs / answeredNodesParticipated); + answeredTargetActiveDocs += (numMissingNodes * answeredTargetActiveDocs / answeredNodesParticipated); + } + return this; + } } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/InterleavedSearchInvoker.java b/container-search/src/main/java/com/yahoo/search/dispatch/InterleavedSearchInvoker.java index 167d780918a..2717b48839e 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/InterleavedSearchInvoker.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/InterleavedSearchInvoker.java @@ -127,7 +127,7 @@ public class InterleavedSearchInvoker extends SearchInvoker implements ResponseM groupingResultAggregator.toAggregatedHit().ifPresent(h -> result.getResult().hits().add(h)); insertNetworkErrors(result.getResult()); - CoverageAggregator adjusted = coverageAggregator.adjustedDegradedCoverage((int)searchCluster.dispatchConfig().searchableCopies(), timeoutHandler); + CoverageAggregator adjusted = coverageAggregator.adjustedDegradedCoverage(redundancyForCoverage(searchCluster.dispatchConfig()), timeoutHandler); result.getResult().setCoverage(adjusted.createCoverage(timeoutHandler, searchCluster.dispatchConfig().computeCoverageFromTargetActiveDocs())); int needed = query.getOffset() + query.getHits(); @@ -138,6 +138,10 @@ public class InterleavedSearchInvoker extends SearchInvoker implements ResponseM return result; } + private int redundancyForCoverage(DispatchConfig config) { + return (int)(config.computeCoverageFromTargetActiveDocs() ? config.redundancy() : config.searchableCopies()); + } + private void insertNetworkErrors(Result result) { // Network errors will be reported as errors only when all nodes fail, otherwise they are just traced boolean asErrors = coverageAggregator.hasNoAnswers(); diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java index 7fe2d2c2d9d..180beb43ee8 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java @@ -21,10 +21,10 @@ import com.yahoo.searchlib.aggregation.MinAggregationResult; import com.yahoo.searchlib.expression.IntegerResultNode; import com.yahoo.searchlib.expression.StringResultNode; import com.yahoo.test.ManualClock; +import com.yahoo.vespa.config.search.DispatchConfig; import org.junit.jupiter.api.Test; import java.io.IOException; -import java.time.Clock; import java.time.Duration; import java.time.Instant; import java.util.ArrayList; @@ -35,7 +35,6 @@ import java.util.List; import java.util.Optional; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; -import java.util.function.ToLongFunction; import java.util.stream.StreamSupport; import static com.yahoo.container.handler.Coverage.DEGRADED_BY_MATCH_PHASE; @@ -56,52 +55,55 @@ public class InterleavedSearchInvokerTest { @Test void requireThatAdaptiveTimeoutsAreNotUsedWithFullCoverageRequirement() throws IOException { SearchCluster cluster = new MockSearchCluster("!", createDispatchConfig(100.0), 1, 3); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 3); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 3)) { - expectedEvents.add(new Event(5000, 100, 0)); - expectedEvents.add(new Event(4900, 100, 1)); - expectedEvents.add(new Event(4800, 100, 2)); + expectedEvents.add(new Event(5000, 100, 0)); + expectedEvents.add(new Event(4900, 100, 1)); + expectedEvents.add(new Event(4800, 100, 2)); - invoker.search(query, null); + invoker.search(query, null); - assertTrue(expectedEvents.isEmpty(), "All test scenario events processed"); + assertTrue(expectedEvents.isEmpty(), "All test scenario events processed"); + } } @Test void requireThatTimeoutsAreNotMarkedAsAdaptive() throws IOException { SearchCluster cluster = new MockSearchCluster("!", createDispatchConfig(100.0), 1, 3); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 3); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 3)) { - expectedEvents.add(new Event(5000, 300, 0)); - expectedEvents.add(new Event(4700, 300, 1)); - expectedEvents.add(null); + expectedEvents.add(new Event(5000, 300, 0)); + expectedEvents.add(new Event(4700, 300, 1)); + expectedEvents.add(null); - Result result = invoker.search(query, null); + Result result = invoker.search(query, null); - assertTrue(expectedEvents.isEmpty(), "All test scenario events processed"); - assertNull(result.hits().getErrorHit(), "Result is not marked as an error"); - var message = findTrace(result, "Backend communication timeout"); - assertTrue(message.isPresent(), "Timeout should be reported in a trace message"); - assertTrue(result.getCoverage(false).isDegradedByTimeout(), "Degradation reason is a normal timeout"); + assertTrue(expectedEvents.isEmpty(), "All test scenario events processed"); + assertNull(result.hits().getErrorHit(), "Result is not marked as an error"); + var message = findTrace(result, "Backend communication timeout"); + assertTrue(message.isPresent(), "Timeout should be reported in a trace message"); + assertTrue(result.getCoverage(false).isDegradedByTimeout(), "Degradation reason is a normal timeout"); + } } @Test void requireThatAdaptiveTimeoutDecreasesTimeoutWhenCoverageIsReached() throws IOException { SearchCluster cluster = new MockSearchCluster("!", createDispatchConfig(50.0), 1, 4); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 4); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 4)) { - expectedEvents.add(new Event(5000, 100, 0)); - expectedEvents.add(new Event(4900, 100, 1)); - expectedEvents.add(new Event(2400, 100, 2)); - expectedEvents.add(new Event(0, 0, null)); + expectedEvents.add(new Event(5000, 100, 0)); + expectedEvents.add(new Event(4900, 100, 1)); + expectedEvents.add(new Event(2400, 100, 2)); + expectedEvents.add(new Event(0, 0, null)); - Result result = invoker.search(query, null); + Result result = invoker.search(query, null); - assertTrue(expectedEvents.isEmpty(), "All test scenario events processed"); - assertNull(result.hits().getErrorHit(), "Result is not marked as an error"); - var message = findTrace(result, "Backend communication timeout"); - assertTrue(message.isPresent(), "Timeout should be reported in a trace message"); - assertTrue(result.getCoverage(false).isDegradedByAdapativeTimeout(), "Degradataion reason is an adaptive timeout"); + assertTrue(expectedEvents.isEmpty(), "All test scenario events processed"); + assertNull(result.hits().getErrorHit(), "Result is not marked as an error"); + var message = findTrace(result, "Backend communication timeout"); + assertTrue(message.isPresent(), "Timeout should be reported in a trace message"); + assertTrue(result.getCoverage(false).isDegradedByAdapativeTimeout(), "Degradataion reason is an adaptive timeout"); + } } @Test @@ -109,20 +111,21 @@ public class InterleavedSearchInvokerTest { SearchCluster cluster = new MockSearchCluster("!", 1, 2); invokers.add(new MockInvoker(0, createCoverage(50155, 50155, 50155, 1, 1, 0))); invokers.add(new MockInvoker(1, createCoverage(49845, 49845, 49845, 1, 1, 0))); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0)) { - expectedEvents.add(new Event(null, 100, 0)); - expectedEvents.add(new Event(null, 200, 1)); + expectedEvents.add(new Event(null, 100, 0)); + expectedEvents.add(new Event(null, 200, 1)); - Result result = invoker.search(query, null); + Result result = invoker.search(query, null); - Coverage cov = result.getCoverage(true); - assertEquals(100000L, cov.getDocs()); - assertEquals(2, cov.getNodes()); - assertTrue(cov.getFull()); - assertEquals(100, cov.getResultPercentage()); - assertEquals(1, cov.getResultSets()); - assertEquals(1, cov.getFullResultSets()); + Coverage cov = result.getCoverage(true); + assertEquals(100000L, cov.getDocs()); + assertEquals(2, cov.getNodes()); + assertTrue(cov.getFull()); + assertEquals(100, cov.getResultPercentage()); + assertEquals(1, cov.getResultSets()); + assertEquals(1, cov.getFullResultSets()); + } } @Test @@ -130,21 +133,22 @@ public class InterleavedSearchInvokerTest { SearchCluster cluster = new MockSearchCluster("!", 1, 2); invokers.add(new MockInvoker(0, createCoverage(10101, 50155, 50155, 1, 1, DEGRADED_BY_MATCH_PHASE))); invokers.add(new MockInvoker(1, createCoverage(13319, 49845, 49845, 1, 1, DEGRADED_BY_MATCH_PHASE))); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0)) { - expectedEvents.add(new Event(null, 100, 0)); - expectedEvents.add(new Event(null, 200, 1)); + expectedEvents.add(new Event(null, 100, 0)); + expectedEvents.add(new Event(null, 200, 1)); - Result result = invoker.search(query, null); + Result result = invoker.search(query, null); - Coverage cov = result.getCoverage(true); - assertEquals(23420L, cov.getDocs()); - assertEquals(2, cov.getNodes()); - assertFalse(cov.getFull()); - assertEquals(23, cov.getResultPercentage()); - assertEquals(1, cov.getResultSets()); - assertEquals(0, cov.getFullResultSets()); - assertTrue(cov.isDegradedByMatchPhase()); + Coverage cov = result.getCoverage(true); + assertEquals(23420L, cov.getDocs()); + assertEquals(2, cov.getNodes()); + assertFalse(cov.getFull()); + assertEquals(23, cov.getResultPercentage()); + assertEquals(1, cov.getResultSets()); + assertEquals(0, cov.getFullResultSets()); + assertTrue(cov.isDegradedByMatchPhase()); + } } @Test @@ -152,21 +156,22 @@ public class InterleavedSearchInvokerTest { SearchCluster cluster = new MockSearchCluster("!", 1, 2); invokers.add(new MockInvoker(0, createCoverage(5000, 50155, 50155, 1, 1, DEGRADED_BY_TIMEOUT))); invokers.add(new MockInvoker(1, createCoverage(4900, 49845, 49845, 1, 1, DEGRADED_BY_TIMEOUT))); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0)) { - expectedEvents.add(new Event(null, 100, 0)); - expectedEvents.add(new Event(null, 200, 1)); + expectedEvents.add(new Event(null, 100, 0)); + expectedEvents.add(new Event(null, 200, 1)); - Result result = invoker.search(query, null); + Result result = invoker.search(query, null); - Coverage cov = result.getCoverage(true); - assertEquals(9900L, cov.getDocs()); - assertEquals(2, cov.getNodes()); - assertFalse(cov.getFull()); - assertEquals(10, cov.getResultPercentage()); - assertEquals(1, cov.getResultSets()); - assertEquals(0, cov.getFullResultSets()); - assertTrue(cov.isDegradedByTimeout()); + Coverage cov = result.getCoverage(true); + assertEquals(9900L, cov.getDocs()); + assertEquals(2, cov.getNodes()); + assertFalse(cov.getFull()); + assertEquals(10, cov.getResultPercentage()); + assertEquals(1, cov.getResultSets()); + assertEquals(0, cov.getFullResultSets()); + assertTrue(cov.isDegradedByTimeout()); + } } @Test @@ -174,22 +179,23 @@ public class InterleavedSearchInvokerTest { SearchCluster cluster = new MockSearchCluster("!", 1, 2); invokers.add(new MockInvoker(0, createCoverage(50155, 50155, 50155, 1, 1, 0))); invokers.add(new MockInvoker(1, createCoverage(49845, 49845, 49845, 1, 1, 0))); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0); - - expectedEvents.add(new Event(null, 100, 0)); - expectedEvents.add(null); - - Result result = invoker.search(query, null); - - Coverage cov = result.getCoverage(true); - assertEquals(50155L, cov.getDocs()); - assertEquals(1, cov.getNodes()); - assertEquals(2, cov.getNodesTried()); - assertFalse(cov.getFull()); - assertEquals(50, cov.getResultPercentage()); - assertEquals(1, cov.getResultSets()); - assertEquals(0, cov.getFullResultSets()); - assertTrue(cov.isDegradedByTimeout()); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0)) { + + expectedEvents.add(new Event(null, 100, 0)); + expectedEvents.add(null); + + Result result = invoker.search(query, null); + + Coverage cov = result.getCoverage(true); + assertEquals(50155L, cov.getDocs()); + assertEquals(1, cov.getNodes()); + assertEquals(2, cov.getNodesTried()); + assertFalse(cov.getFull()); + assertEquals(50, cov.getResultPercentage()); + assertEquals(1, cov.getResultSets()); + assertEquals(0, cov.getFullResultSets()); + assertTrue(cov.isDegradedByTimeout()); + } } static class MetaHit extends Hit { @@ -209,24 +215,25 @@ public class InterleavedSearchInvokerTest { private static final List<Double> B5Aux = Arrays.asList(9.0,8.0,-3.0,7.0,6.0,1.0, -1.0); private void validateThatTopKProbabilityOverrideTakesEffect(Double topKProbability, int expectedK, Group group) throws IOException { - InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, group); - query.setHits(8); - query.properties().set(Dispatcher.topKProbability, topKProbability); - SearchInvoker [] invokers = invoker.invokers().toArray(new SearchInvoker[0]); - Result result = invoker.search(query, null); - assertEquals(2, invokers.length); - assertEquals(expectedK, ((MockInvoker)invokers[0]).hitsRequested); - assertEquals(8, result.hits().size()); - assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(9.0, result.hits().get(1).getRelevance().getScore(), DELTA); - assertEquals(8.5, result.hits().get(2).getRelevance().getScore(), DELTA); - assertEquals(8.0, result.hits().get(3).getRelevance().getScore(), DELTA); - assertEquals(7.5, result.hits().get(4).getRelevance().getScore(), DELTA); - assertEquals(7.0, result.hits().get(5).getRelevance().getScore(), DELTA); - assertEquals(6.0, result.hits().get(6).getRelevance().getScore(), DELTA); - assertEquals(3.0, result.hits().get(7).getRelevance().getScore(), DELTA); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(8, result.getQuery().getHits()); + try (InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, group)) { + query.setHits(8); + query.properties().set(Dispatcher.topKProbability, topKProbability); + SearchInvoker[] invokers = invoker.invokers().toArray(new SearchInvoker[0]); + Result result = invoker.search(query, null); + assertEquals(2, invokers.length); + assertEquals(expectedK, ((MockInvoker) invokers[0]).hitsRequested); + assertEquals(8, result.hits().size()); + assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(9.0, result.hits().get(1).getRelevance().getScore(), DELTA); + assertEquals(8.5, result.hits().get(2).getRelevance().getScore(), DELTA); + assertEquals(8.0, result.hits().get(3).getRelevance().getScore(), DELTA); + assertEquals(7.5, result.hits().get(4).getRelevance().getScore(), DELTA); + assertEquals(7.0, result.hits().get(5).getRelevance().getScore(), DELTA); + assertEquals(6.0, result.hits().get(6).getRelevance().getScore(), DELTA); + assertEquals(3.0, result.hits().get(7).getRelevance().getScore(), DELTA); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(8, result.getQuery().getHits()); + } } @Test @@ -261,68 +268,74 @@ public class InterleavedSearchInvokerTest { @Test void requireThatMergeOfConcreteHitsObeySorting() throws IOException { - InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, new Group(0, List.of())); - query.setHits(12); - Result result = invoker.search(query, null); - assertEquals(10, result.hits().size()); - assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(1.0, result.hits().get(9).getRelevance().getScore(), DELTA); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(12, result.getQuery().getHits()); - - invoker = createInterLeavedTestInvoker(B5, A5, new Group(0, List.of())); - result = invoker.search(query, null); - assertEquals(10, result.hits().size()); - assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(1.0, result.hits().get(9).getRelevance().getScore(), DELTA); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(12, result.getQuery().getHits()); + try (InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, new Group(0, List.of()))) { + query.setHits(12); + Result result = invoker.search(query, null); + assertEquals(10, result.hits().size()); + assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(1.0, result.hits().get(9).getRelevance().getScore(), DELTA); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(12, result.getQuery().getHits()); + } + + try ( InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(B5, A5, new Group(0, List.of()))) { + Result result = invoker.search(query, null); + assertEquals(10, result.hits().size()); + assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(1.0, result.hits().get(9).getRelevance().getScore(), DELTA); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(12, result.getQuery().getHits()); + } } @Test void requireThatMergeOfConcreteHitsObeyOffset() throws IOException { - InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, new Group(0, List.of())); - query.setHits(3); - query.setOffset(5); - Result result = invoker.search(query, null); - assertEquals(3, result.hits().size()); - assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(3, result.getQuery().getHits()); - - invoker = createInterLeavedTestInvoker(B5, A5, new Group(0, List.of())); - query.setOffset(5); - result = invoker.search(query, null); - assertEquals(3, result.hits().size()); - assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(3, result.getQuery().getHits()); + try (InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, new Group(0, List.of()))) { + query.setHits(3); + query.setOffset(5); + Result result = invoker.search(query, null); + assertEquals(3, result.hits().size()); + assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(3, result.getQuery().getHits()); + } + + try (InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(B5, A5, new Group(0, List.of()))) { + query.setOffset(5); + Result result = invoker.search(query, null); + assertEquals(3, result.hits().size()); + assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(3, result.getQuery().getHits()); + } } @Test void requireThatMergeOfConcreteHitsObeyOffsetWithAuxilliaryStuff() throws IOException { - InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5Aux, B5Aux, new Group(0, List.of())); - query.setHits(3); - query.setOffset(5); - Result result = invoker.search(query, null); - assertEquals(7, result.hits().size()); - assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); - assertTrue(result.hits().get(3) instanceof MetaHit); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(3, result.getQuery().getHits()); - - invoker = createInterLeavedTestInvoker(B5Aux, A5Aux, new Group(0, List.of())); - query.setOffset(5); - result = invoker.search(query, null); - assertEquals(7, result.hits().size()); - assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); - assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); - assertTrue(result.hits().get(3) instanceof MetaHit); - assertEquals(0, result.getQuery().getOffset()); - assertEquals(3, result.getQuery().getHits()); + try (InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5Aux, B5Aux, new Group(0, List.of()))) { + query.setHits(3); + query.setOffset(5); + Result result = invoker.search(query, null); + assertEquals(7, result.hits().size()); + assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); + assertTrue(result.hits().get(3) instanceof MetaHit); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(3, result.getQuery().getHits()); + } + + try (InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(B5Aux, A5Aux, new Group(0, List.of()))) { + query.setOffset(5); + Result result = invoker.search(query, null); + assertEquals(7, result.hits().size()); + assertEquals(7.0, result.hits().get(0).getRelevance().getScore(), DELTA); + assertEquals(3.0, result.hits().get(2).getRelevance().getScore(), DELTA); + assertTrue(result.hits().get(3) instanceof MetaHit); + assertEquals(0, result.getQuery().getOffset()); + assertEquals(3, result.getQuery().getHits()); + } } @Test @@ -350,12 +363,15 @@ public class InterleavedSearchInvokerTest { .addAggregationResult(new MinAggregationResult().setMin(new IntegerResultNode(6)).setTag(3)))); invokers.add(new MockInvoker(0).setHits(List.of(new GroupingListHit(List.of(grouping2))))); - InterleavedSearchInvoker invoker = new InterleavedSearchInvoker(Timer.monotonic, invokers, cluster, new Group(0, List.of()), Collections.emptySet()); - invoker.responseAvailable(invokers.get(0)); - invoker.responseAvailable(invokers.get(1)); - Result result = invoker.search(query, null); - assertEquals(1, ((GroupingListHit) result.hits().get(0)).getGroupingList().size()); - + try (InterleavedSearchInvoker invoker = new InterleavedSearchInvoker(Timer.monotonic, invokers, cluster, new Group(0, List.of()), Collections.emptySet())) { + invoker.responseAvailable(invokers.get(0)); + invoker.responseAvailable(invokers.get(1)); + Result result = invoker.search(query, null); + assertEquals(1, ((GroupingListHit) result.hits().get(0)).getGroupingList().size()); + } + for (SearchInvoker invoker : invokers) { + invoker.close(); + } } private static InterleavedSearchInvoker createInterLeavedTestInvoker(List<Double> a, List<Double> b, Group group) { @@ -384,29 +400,46 @@ public class InterleavedSearchInvokerTest { return hits; } - @Test - void requireCorrectCoverageCalculationWhenDegradedCoverageIsExpected() throws IOException { - SearchCluster cluster = new MockSearchCluster("!", 1, 2); - invokers.add(new MockInvoker(0, createCoverage(50155, 50155, 50155, 1, 1, 0))); + void verifyCorrectCoverageCalculationWhenDegradedCoverageIsExpected(DispatchConfig dispatchConfig, int expectedCoverage) throws IOException { + SearchCluster cluster = new MockSearchCluster("!", dispatchConfig, 1, 2); + invokers.add(new MockInvoker(0, createCoverage(50155, 50155, 60000, 1, 1, 0))); Coverage errorCoverage = new Coverage(0, 0, 0); errorCoverage.setNodesTried(1); invokers.add(new SearchErrorInvoker(ErrorMessage.createBackendCommunicationError("node is down"), errorCoverage)); - SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0); - - expectedEvents.add(new Event(null, 1, 1)); - expectedEvents.add(new Event(null, 100, 0)); - - Result result = invoker.search(query, null); - - Coverage cov = result.getCoverage(true); - assertEquals(50155L, cov.getDocs()); - assertEquals(1, cov.getNodes()); - assertEquals(2, cov.getNodesTried()); - assertFalse(cov.getFull()); - assertEquals(50, cov.getResultPercentage()); - assertEquals(1, cov.getResultSets()); - assertEquals(0, cov.getFullResultSets()); - assertTrue(cov.isDegradedByTimeout()); + try (SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0)) { + + expectedEvents.add(new Event(null, 1, 1)); + expectedEvents.add(new Event(null, 100, 0)); + + Result result = invoker.search(query, null); + + Coverage cov = result.getCoverage(true); + assertEquals(50155L, cov.getDocs()); + assertEquals(1, cov.getNodes()); + assertEquals(2, cov.getNodesTried()); + assertFalse(cov.getFull()); + assertEquals(expectedCoverage, cov.getResultPercentage()); + assertEquals(1, cov.getResultSets()); + assertEquals(0, cov.getFullResultSets()); + assertTrue(cov.isDegradedByTimeout()); + } + } + @Test + void requireCorrectCoverageCalculationWhenDegradedCoverageIsExpectedUsingActiveDocs() throws IOException { + verifyCorrectCoverageCalculationWhenDegradedCoverageIsExpected(MockSearchCluster.createDispatchConfig(100.0, List.of()) + .searchableCopies(1) + .redundancy(2) + .build(), + 50); + } + @Test + void requireCorrectCoverageCalculationWhenDegradedCoverageIsExpectedUsingTargetActiveDocs() throws IOException { + verifyCorrectCoverageCalculationWhenDegradedCoverageIsExpected(MockSearchCluster.createDispatchConfig(100.0, List.of()) + .searchableCopies(1) + .redundancy(1) + .computeCoverageFromTargetActiveDocs(true) + .build(), + 42); } private InterleavedSearchInvoker createInterleavedInvoker(SearchCluster searchCluster, Group group, int numInvokers) { diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java b/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java index a076b929ad7..7a11e906293 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java @@ -105,14 +105,14 @@ public class MockSearchCluster extends SearchCluster { return createDispatchConfig(100.0, nodes); } public static DispatchConfig createDispatchConfig(List<Node> nodes) { - return createDispatchConfig(100.0, nodes); + return createDispatchConfig(100.0, nodes).build(); } public static DispatchConfig createDispatchConfig(double minSearchCoverage, Node... nodes) { - return createDispatchConfig(minSearchCoverage, Arrays.asList(nodes)); + return createDispatchConfig(minSearchCoverage, Arrays.asList(nodes)).build(); } - public static DispatchConfig createDispatchConfig(double minSearchCoverage, List<Node> nodes) { + public static DispatchConfig.Builder createDispatchConfig(double minSearchCoverage, List<Node> nodes) { DispatchConfig.Builder builder = new DispatchConfig.Builder(); builder.minActivedocsPercentage(88.0); builder.minSearchCoverage(minSearchCoverage); @@ -125,7 +125,7 @@ public class MockSearchCluster extends SearchCluster { for (Node n : nodes) { builder.node(new DispatchConfig.Node.Builder().key(n.key()).host(n.hostname()).port(port++).group(n.group())); } - return new DispatchConfig(builder); + return builder; } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java index 15ba96caa70..328be3b32b8 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java @@ -56,7 +56,7 @@ public class NodePatcher { private static final String WANT_TO_RETIRE = "wantToRetire"; private static final String WANT_TO_DEPROVISION = "wantToDeprovision"; private static final String WANT_TO_REBUILD = "wantToRebuild"; - private static final Set<String> RECURSIVE_FIELDS = Set.of(WANT_TO_RETIRE); + private static final Set<String> RECURSIVE_FIELDS = Set.of(WANT_TO_RETIRE, WANT_TO_DEPROVISION); private static final Set<String> IP_CONFIG_FIELDS = Set.of("ipAddresses", "additionalIpAddresses", "additionalHostnames"); diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/documentstoreadapter.cpp b/searchcore/src/vespa/searchcore/proton/docsummary/documentstoreadapter.cpp index a62bd155b87..6d717f7f128 100644 --- a/searchcore/src/vespa/searchcore/proton/docsummary/documentstoreadapter.cpp +++ b/searchcore/src/vespa/searchcore/proton/docsummary/documentstoreadapter.cpp @@ -2,7 +2,6 @@ #include "documentstoreadapter.h" #include <vespa/searchsummary/docsummary/docsum_store_document.h> -#include <vespa/searchsummary/docsummary/summaryfieldconverter.h> #include <vespa/document/fieldvalue/document.h> #include <vespa/document/fieldvalue/stringfieldvalue.h> #include <vespa/eval/eval/value_codec.h> diff --git a/searchsummary/src/tests/docsummary/matched_elements_filter/matched_elements_filter_test.cpp b/searchsummary/src/tests/docsummary/matched_elements_filter/matched_elements_filter_test.cpp index f0dd4acfb32..b165542e2e2 100644 --- a/searchsummary/src/tests/docsummary/matched_elements_filter/matched_elements_filter_test.cpp +++ b/searchsummary/src/tests/docsummary/matched_elements_filter/matched_elements_filter_test.cpp @@ -22,7 +22,6 @@ #include <vespa/searchsummary/docsummary/matched_elements_filter_dfw.h> #include <vespa/searchsummary/docsummary/resultclass.h> #include <vespa/searchsummary/docsummary/resultconfig.h> -#include <vespa/searchsummary/docsummary/summaryfieldconverter.h> #include <vespa/searchsummary/test/slime_value.h> #include <vespa/vespalib/data/slime/slime.h> #include <vespa/vespalib/gtest/gtest.h> diff --git a/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt b/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt index 51c8afe4250..e69752f3b81 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt +++ b/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt @@ -38,5 +38,4 @@ vespa_add_library(searchsummary_docsummary OBJECT struct_fields_resolver.cpp struct_map_attribute_combiner_dfw.cpp summaryfeaturesdfw.cpp - summaryfieldconverter.cpp ) diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsum_store_document.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsum_store_document.cpp index 35db818ac58..f701b4796c1 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsum_store_document.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/docsum_store_document.cpp @@ -2,7 +2,7 @@ #include "docsum_store_document.h" #include "annotation_converter.h" -#include "summaryfieldconverter.h" +#include "slime_filler.h" #include <vespa/document/base/exceptions.h> #include <vespa/document/datatype/datatype.h> #include <vespa/document/fieldvalue/document.h> @@ -41,7 +41,7 @@ DocsumStoreDocument::insert_summary_field(const vespalib::string& field_name, ve { auto field_value = get_field_value(field_name); if (field_value) { - SummaryFieldConverter::insert_summary_field(*field_value, inserter); + SlimeFiller::insert_summary_field(*field_value, inserter); } } @@ -51,7 +51,7 @@ DocsumStoreDocument::insert_juniper_field(const vespalib::string& field_name, ve auto field_value = get_field_value(field_name); if (field_value) { AnnotationConverter stacked_converter(converter); - SummaryFieldConverter::insert_juniper_field(*field_value, inserter, stacked_converter); + SlimeFiller::insert_juniper_field(*field_value, inserter, stacked_converter); } } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/matched_elements_filter_dfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/matched_elements_filter_dfw.cpp index 1a029cfd16f..4f74ffd894c 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/matched_elements_filter_dfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/matched_elements_filter_dfw.cpp @@ -3,8 +3,8 @@ #include "matched_elements_filter_dfw.h" #include "docsumstate.h" #include "i_docsum_store_document.h" +#include "slime_filler.h" #include "struct_fields_resolver.h" -#include "summaryfieldconverter.h" #include <vespa/document/fieldvalue/document.h> #include <vespa/document/fieldvalue/literalfieldvalue.h> #include <vespa/searchcommon/attribute/iattributecontext.h> @@ -68,7 +68,7 @@ MatchedElementsFilterDFW::insertField(uint32_t docid, const IDocsumStoreDocument { auto field_value = doc->get_field_value(_input_field_name); if (field_value) { - SummaryFieldConverter::insert_summary_field_with_filter(*field_value, target, get_matching_elements(docid, state)); + SlimeFiller::insert_summary_field_with_filter(*field_value, target, get_matching_elements(docid, state)); } } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp index 94774c1bee4..06cb363b61f 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "slime_filler.h" +#include "check_undefined_value_visitor.h" #include "i_juniper_converter.h" #include "i_string_field_converter.h" #include "resultconfig.h" @@ -341,4 +342,37 @@ SlimeFiller::visit(const ReferenceFieldValue& value) : vespalib::string())); } +void +SlimeFiller::insert_summary_field(const FieldValue& value, vespalib::slime::Inserter& inserter) +{ + CheckUndefinedValueVisitor check_undefined; + value.accept(check_undefined); + if (!check_undefined.is_undefined()) { + SlimeFiller visitor(inserter); + value.accept(visitor); + } +} + +void +SlimeFiller::insert_summary_field_with_filter(const FieldValue& value, vespalib::slime::Inserter& inserter, const std::vector<uint32_t>& matching_elems) +{ + CheckUndefinedValueVisitor check_undefined; + value.accept(check_undefined); + if (!check_undefined.is_undefined()) { + SlimeFiller visitor(inserter, &matching_elems); + value.accept(visitor); + } +} + +void +SlimeFiller::insert_juniper_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter, IStringFieldConverter& converter) +{ + CheckUndefinedValueVisitor check_undefined; + value.accept(check_undefined); + if (!check_undefined.is_undefined()) { + SlimeFiller visitor(inserter, &converter, nullptr); + value.accept(visitor); + } +} + } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h index a81a20814c4..2f924de8af4 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h @@ -6,6 +6,8 @@ #include <cstdint> #include <vector> +namespace document { class FieldValue; } + namespace vespalib::slime { struct Inserter; } namespace search::docsummary { @@ -55,6 +57,14 @@ public: SlimeFiller(vespalib::slime::Inserter& inserter, const std::vector<uint32_t>* matching_elems); SlimeFiller(vespalib::slime::Inserter& inserter, IStringFieldConverter* string_converter, const SlimeFillerFilter* filter); ~SlimeFiller() override; + + static void insert_summary_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter); + + /** + * Insert the given field value, but only the elements that are contained in the matching_elems vector. + */ + static void insert_summary_field_with_filter(const document::FieldValue& value, vespalib::slime::Inserter& inserter, const std::vector<uint32_t>& matching_elems); + static void insert_juniper_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter, IStringFieldConverter& converter); }; } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp b/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp deleted file mode 100644 index dd5a59e46af..00000000000 --- a/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include "summaryfieldconverter.h" -#include "check_undefined_value_visitor.h" -#include "slime_filler.h" -#include <vespa/document/fieldvalue/fieldvalue.h> - -using document::FieldValue; - -namespace search::docsummary { - -void -SummaryFieldConverter::insert_summary_field(const FieldValue& value, vespalib::slime::Inserter& inserter) -{ - CheckUndefinedValueVisitor check_undefined; - value.accept(check_undefined); - if (!check_undefined.is_undefined()) { - SlimeFiller visitor(inserter); - value.accept(visitor); - } -} - -void -SummaryFieldConverter::insert_summary_field_with_filter(const FieldValue& value, vespalib::slime::Inserter& inserter, const std::vector<uint32_t>& matching_elems) -{ - CheckUndefinedValueVisitor check_undefined; - value.accept(check_undefined); - if (!check_undefined.is_undefined()) { - SlimeFiller visitor(inserter, &matching_elems); - value.accept(visitor); - } -} - -void -SummaryFieldConverter::insert_juniper_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter, IStringFieldConverter& converter) -{ - CheckUndefinedValueVisitor check_undefined; - value.accept(check_undefined); - if (!check_undefined.is_undefined()) { - SlimeFiller visitor(inserter, &converter, nullptr); - value.accept(visitor); - } -} - -} diff --git a/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.h b/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.h deleted file mode 100644 index ce3bf80b365..00000000000 --- a/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.h +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#pragma once - -#include <cstdint> -#include <vector> - -namespace document { class FieldValue; } - -namespace vespalib::slime { struct Inserter; } - -namespace search::docsummary { - -class IStringFieldConverter; - -/** - * This class converts a summary field for docsum fetching. - */ -class SummaryFieldConverter -{ -public: - static void insert_summary_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter); - /** - * Insert the given field value, but only the elements that are contained in the matching_elems vector. - */ - static void insert_summary_field_with_filter(const document::FieldValue& value, vespalib::slime::Inserter& inserter, const std::vector<uint32_t>& matching_elems); - static void insert_juniper_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter, IStringFieldConverter& converter); -}; - -} - diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp b/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp index ad054aadd9c..7fbef6307b2 100644 --- a/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp +++ b/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp @@ -8,7 +8,6 @@ #include <vespa/searchsummary/docsummary/i_string_field_converter.h> #include <vespa/searchsummary/docsummary/slime_filler.h> #include <vespa/searchsummary/docsummary/slime_filler_filter.h> -#include <vespa/searchsummary/docsummary/summaryfieldconverter.h> #include <vespa/document/base/exceptions.h> #include <vespa/document/datatype/datatype.h> #include <vespa/document/fieldvalue/stringfieldvalue.h> @@ -242,7 +241,7 @@ DocsumStoreVsmDocument::insert_summary_field(const vespalib::string& field_name, auto value(field.getDataType().createFieldValue()); if (value) { if (_document->getValue(field, *value)) { - SummaryFieldConverter::insert_summary_field(*value, inserter); + SlimeFiller::insert_summary_field(*value, inserter); } } } catch (document::FieldNotFoundException&) { @@ -267,7 +266,7 @@ DocsumStoreVsmDocument::insert_juniper_field(const vespalib::string& field_name, // Markup for juniper has already been added due to FLATTENJUNIPER command in vsm summary config. } SnippetModifierJuniperConverter string_converter(converter, modifier); - SummaryFieldConverter::insert_juniper_field(*field_value, inserter, string_converter); + SlimeFiller::insert_juniper_field(*field_value, inserter, string_converter); } } @@ -445,7 +444,7 @@ DocsumFilter::insert_summary_field(uint32_t entry_idx, const Document& doc, vesp const DocsumFieldSpec::FieldIdentifier & fieldId = field_spec.getInputFields()[0]; const document::FieldValue* field_value = doc.getField(fieldId.getId()); if (field_value != nullptr) { - SummaryFieldConverter::insert_summary_field(*field_value, inserter); + SlimeFiller::insert_summary_field(*field_value, inserter); } } else if (field_spec.getInputFields().empty() && field_spec.getCommand() == VsmsummaryConfig::Fieldmap::Command::NONE) { } else { |