diff options
107 files changed, 1037 insertions, 508 deletions
diff --git a/README.md b/README.md index 8d3153b73a2..e37d5d3c68b 100644 --- a/README.md +++ b/README.md @@ -66,11 +66,11 @@ Some suggested improvements with pointers to code are in [TODO.md](TODO.md). ### Development environment -C++ and Java building is supported on CentOS Stream 8. +C++ and Java building is supported on AlmaLinux 8. The Java source can also be built on any platform having Java 17 and Maven installed. Use the following guide to set up a complete development environment using Docker for building Vespa, running unit tests and running system tests: -[Vespa development on CentOS Stream 8](https://github.com/vespa-engine/docker-image-dev#vespa-development-on-centos-stream-8). +[Vespa development on AlmaLinux 8](https://github.com/vespa-engine/docker-image-dev#vespa-development-on-almalinux-8). ### Build Java modules diff --git a/application-model/src/main/java/com/yahoo/vespa/applicationmodel/InfrastructureApplication.java b/application-model/src/main/java/com/yahoo/vespa/applicationmodel/InfrastructureApplication.java index 560c3d169d3..72a278b248a 100644 --- a/application-model/src/main/java/com/yahoo/vespa/applicationmodel/InfrastructureApplication.java +++ b/application-model/src/main/java/com/yahoo/vespa/applicationmodel/InfrastructureApplication.java @@ -5,6 +5,7 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.NodeType; import java.util.List; +import java.util.Optional; import java.util.stream.Stream; /** @@ -37,6 +38,16 @@ public enum InfrastructureApplication { .orElseThrow(() -> new IllegalArgumentException("No application associated with " + nodeType)); } + public static Optional<InfrastructureApplication> ofOptional(ApplicationId applicationId) { + for (var application : values()) { + if (application.id.equals(applicationId)) { + return Optional.of(application); + } + } + + return Optional.empty(); + } + InfrastructureApplication(String name, NodeType nodeType) { this.id = ApplicationId.from(TenantId.HOSTED_VESPA.value(), name, "default"); this.nodeType = nodeType; diff --git a/application/abi-spec.json b/application/abi-spec.json index 95a9d2a524a..27b01d02b5f 100644 --- a/application/abi-spec.json +++ b/application/abi-spec.json @@ -324,6 +324,7 @@ "public com.yahoo.application.container.Processing processing()", "public com.yahoo.application.container.DocumentProcessing documentProcessing()", "public com.yahoo.component.provider.ComponentRegistry components()", + "public com.yahoo.component.provider.ComponentRegistry handlers()", "public com.yahoo.application.container.handler.Response handleRequest(com.yahoo.application.container.handler.Request)", "public void close()" ], @@ -450,11 +451,13 @@ "public void <init>(java.lang.String, java.lang.String)", "public void <init>(java.lang.String, byte[], com.yahoo.application.container.handler.Request$Method)", "public void <init>(java.lang.String, byte[], com.yahoo.application.container.handler.Request$Method, java.security.Principal)", + "public void <init>(java.lang.String, byte[], com.yahoo.application.container.handler.Request$Method, java.security.Principal, java.net.SocketAddress)", "public void <init>(java.lang.String, java.lang.String, com.yahoo.application.container.handler.Request$Method)", "public com.yahoo.application.container.handler.Headers getHeaders()", "public byte[] getBody()", "public java.lang.String getUri()", "public java.util.Map getAttributes()", + "public java.util.Optional remoteAddress()", "public java.lang.String toString()", "public com.yahoo.application.container.handler.Request$Method getMethod()", "public java.util.Optional getUserPrincipal()" diff --git a/application/src/main/java/com/yahoo/application/container/JDisc.java b/application/src/main/java/com/yahoo/application/container/JDisc.java index 162a5f343a1..3223c8d31c9 100644 --- a/application/src/main/java/com/yahoo/application/container/JDisc.java +++ b/application/src/main/java/com/yahoo/application/container/JDisc.java @@ -169,6 +169,9 @@ public final class JDisc implements AutoCloseable { return container.getComponentRegistry(); } + /** @return registry of all request handlers configured */ + public ComponentRegistry<RequestHandler> handlers() { return container.getRequestHandlerRegistry(); } + /** * Handles the given {@link com.yahoo.application.container.handler.Request} by passing it to the {@link RequestHandler} * that is bound to the request's URI. diff --git a/application/src/main/java/com/yahoo/application/container/SynchronousRequestResponseHandler.java b/application/src/main/java/com/yahoo/application/container/SynchronousRequestResponseHandler.java index c54b3f60cf9..1b4862c75c0 100644 --- a/application/src/main/java/com/yahoo/application/container/SynchronousRequestResponseHandler.java +++ b/application/src/main/java/com/yahoo/application/container/SynchronousRequestResponseHandler.java @@ -90,6 +90,7 @@ final class SynchronousRequestResponseHandler { URI.create(request.getUri()), com.yahoo.jdisc.http.HttpRequest.Method.valueOf(request.getMethod().name())); request.getUserPrincipal().ifPresent(httpRequest::setUserPrincipal); + request.remoteAddress().ifPresent(httpRequest::setRemoteAddress); discRequest = httpRequest; } else { discRequest = new com.yahoo.jdisc.Request(currentContainer, URI.create(request.getUri())); diff --git a/application/src/main/java/com/yahoo/application/container/handler/Request.java b/application/src/main/java/com/yahoo/application/container/handler/Request.java index b6dff44269b..d877258cb15 100644 --- a/application/src/main/java/com/yahoo/application/container/handler/Request.java +++ b/application/src/main/java/com/yahoo/application/container/handler/Request.java @@ -3,6 +3,7 @@ package com.yahoo.application.container.handler; import com.yahoo.api.annotations.Beta; +import java.net.SocketAddress; import java.nio.charset.StandardCharsets; import java.security.Principal; import java.util.Map; @@ -24,6 +25,7 @@ public class Request { private final Method method; private final Map<String, Object> attributes = new ConcurrentHashMap<>(); private final Principal userPrincipal; + private final SocketAddress remoteAddress; /** * Creates a Request with an empty body. @@ -73,10 +75,24 @@ public class Request { * @param principal the user principal of the request */ public Request(String uri, byte[] body, Method method, Principal principal) { + this(uri, body, method, principal, null); + } + + /** + * Creates a Request with a message body, method and user principal. + * + * @param uri the URI of the request + * @param body the body of the request + * @param method the method of the request + * @param principal the user principal of the request + * @param remoteAddress the remote address of the request + */ + public Request(String uri, byte[] body, Method method, Principal principal, SocketAddress remoteAddress) { this.uri = uri; this.body = body; this.method = method; this.userPrincipal = principal; + this.remoteAddress = remoteAddress; } /** @@ -122,6 +138,8 @@ public class Request { return attributes; } + public Optional<SocketAddress> remoteAddress() { return Optional.ofNullable(remoteAddress); } + @Override public String toString() { String bodyStr = (body == null || body.length == 0) ? "[empty]" : "[omitted]"; diff --git a/client/go/internal/cli/cmd/cert.go b/client/go/internal/cli/cmd/cert.go index 1cc50b1faea..9668e78bd1c 100644 --- a/client/go/internal/cli/cmd/cert.go +++ b/client/go/internal/cli/cmd/cert.go @@ -153,7 +153,7 @@ func doCertAdd(cli *CLI, overwriteCertificate bool, args []string) error { if err != nil { return err } - pkg, err := cli.applicationPackageFrom(args, false) + pkg, err := cli.applicationPackageFrom(args, vespa.PackageOptions{}) if err != nil { return err } diff --git a/client/go/internal/cli/cmd/config.go b/client/go/internal/cli/cmd/config.go index cfadc6d32c5..a656279549a 100644 --- a/client/go/internal/cli/cmd/config.go +++ b/client/go/internal/cli/cmd/config.go @@ -152,7 +152,7 @@ $ vespa config set --local wait 600 config := cli.config if localArg { // Need an application package in working directory to allow local configuration - if _, err := cli.applicationPackageFrom(nil, false); err != nil { + if _, err := cli.applicationPackageFrom(nil, vespa.PackageOptions{}); err != nil { return fmt.Errorf("failed to write local configuration: %w", err) } config = cli.config.local @@ -188,7 +188,7 @@ $ vespa config unset --local application RunE: func(cmd *cobra.Command, args []string) error { config := cli.config if localArg { - if _, err := cli.applicationPackageFrom(nil, false); err != nil { + if _, err := cli.applicationPackageFrom(nil, vespa.PackageOptions{}); err != nil { return fmt.Errorf("failed to write local configuration: %w", err) } config = cli.config.local diff --git a/client/go/internal/cli/cmd/deploy.go b/client/go/internal/cli/cmd/deploy.go index aee26975901..8806a21c9fc 100644 --- a/client/go/internal/cli/cmd/deploy.go +++ b/client/go/internal/cli/cmd/deploy.go @@ -51,7 +51,7 @@ $ vespa deploy -t cloud -z perf.aws-us-east-1c`, DisableAutoGenTag: true, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { - pkg, err := cli.applicationPackageFrom(args, true) + pkg, err := cli.applicationPackageFrom(args, vespa.PackageOptions{Compiled: true}) if err != nil { return err } @@ -113,7 +113,7 @@ func newPrepareCmd(cli *CLI) *cobra.Command { DisableAutoGenTag: true, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { - pkg, err := cli.applicationPackageFrom(args, true) + pkg, err := cli.applicationPackageFrom(args, vespa.PackageOptions{Compiled: true}) if err != nil { return fmt.Errorf("could not find application package: %w", err) } diff --git a/client/go/internal/cli/cmd/feed.go b/client/go/internal/cli/cmd/feed.go index 1a32ac7110d..89e13a4673c 100644 --- a/client/go/internal/cli/cmd/feed.go +++ b/client/go/internal/cli/cmd/feed.go @@ -64,10 +64,36 @@ func newFeedCmd(cli *CLI) *cobra.Command { This command can be used to feed large amounts of documents to a Vespa cluster efficiently. -The contents of JSON-FILE must be either a JSON array or JSON objects separated by +The contents of json-file must be either a JSON array or JSON objects separated by newline (JSONL). -If JSON-FILE is a single dash ('-'), documents will be read from standard input. +If json-file is a single dash ('-'), documents will be read from standard input. + +Once feeding completes, metrics of the feed session are printed to standard out +in a JSON format: + +- feeder.operation.count: Number of operations passed to the feeder by the user, + not counting retries. +- feeder.seconds: Total time spent feeding. +- feeder.ok.count: Number of successful operations. +- feeder.ok.rate: Number of successful operations per second. +- feeder.error.count: Number of network errors (transport layer). +- feeder.inflight.count: Number of operations currently being sent. +- http.request.count: Number of HTTP requests made, including retries. +- http.request.bytes: Number of bytes sent. +- http.request.MBps: Request throughput measured in MB/s. This is the raw + operation throughput, and not the network throughput, + I.e. using compression does not affect this number. +- http.exception.count: Same as feeder.error.count. Present for compatiblity + with vespa-feed-client. +- http.response.count: Number of HTTP responses received. +- http.response.bytes: Number of bytes received. +- http.response.MBps: Response throughput measured in MB/s. +- http.response.error.count: Number of non-OK HTTP responses received. +- http.response.latency.millis.min: Lowest latency of a successful operation. +- http.response.latency.millis.avg: Average latency of successful operations. +- http.response.latency.millis.max: Highest latency of a successful operation. +- http.response.code.counts: Number of responses grouped by their HTTP code. `, Example: `$ vespa feed docs.jsonl moredocs.json $ cat docs.jsonl | vespa feed -`, @@ -244,6 +270,7 @@ type number float32 func (n number) MarshalJSON() ([]byte, error) { return []byte(fmt.Sprintf("%.3f", n)), nil } type feedSummary struct { + Operations int64 `json:"feeder.operation.count"` Seconds number `json:"feeder.seconds"` SuccessCount int64 `json:"feeder.ok.count"` SuccessRate number `json:"feeder.ok.rate"` @@ -272,6 +299,7 @@ func mbps(bytes int64, duration time.Duration) float64 { func writeSummaryJSON(w io.Writer, stats document.Stats, duration time.Duration) error { summary := feedSummary{ + Operations: stats.Operations, Seconds: number(duration.Seconds()), SuccessCount: stats.Successful(), SuccessRate: number(float64(stats.Successful()) / math.Max(1, duration.Seconds())), diff --git a/client/go/internal/cli/cmd/feed_test.go b/client/go/internal/cli/cmd/feed_test.go index 1cf9a6aba3c..daf649a0fd1 100644 --- a/client/go/internal/cli/cmd/feed_test.go +++ b/client/go/internal/cli/cmd/feed_test.go @@ -48,6 +48,7 @@ func TestFeed(t *testing.T) { assert.Equal(t, "", stderr.String()) want := `{ + "feeder.operation.count": 2, "feeder.seconds": 5.000, "feeder.ok.count": 2, "feeder.ok.rate": 0.400, @@ -122,6 +123,7 @@ func TestFeedInvalid(t *testing.T) { require.NotNil(t, cli.Run("feed", "-t", "http://127.0.0.1:8080", jsonFile)) want := `{ + "feeder.operation.count": 1, "feeder.seconds": 3.000, "feeder.ok.count": 1, "feeder.ok.rate": 0.333, diff --git a/client/go/internal/cli/cmd/prod.go b/client/go/internal/cli/cmd/prod.go index 9e78b299e4b..74e4b4c4a1c 100644 --- a/client/go/internal/cli/cmd/prod.go +++ b/client/go/internal/cli/cmd/prod.go @@ -57,7 +57,7 @@ https://cloud.vespa.ai/en/reference/deployment`, if err != nil { return err } - pkg, err := cli.applicationPackageFrom(args, false) + pkg, err := cli.applicationPackageFrom(args, vespa.PackageOptions{SourceOnly: true}) if err != nil { return err } @@ -143,7 +143,7 @@ $ vespa prod deploy`, // TODO: Add support for hosted return fmt.Errorf("prod deploy does not support %s target", target.Type()) } - pkg, err := cli.applicationPackageFrom(args, true) + pkg, err := cli.applicationPackageFrom(args, vespa.PackageOptions{Compiled: true}) if err != nil { return err } diff --git a/client/go/internal/cli/cmd/root.go b/client/go/internal/cli/cmd/root.go index 068a7ed90b6..4d1a7cf6f89 100644 --- a/client/go/internal/cli/cmd/root.go +++ b/client/go/internal/cli/cmd/root.go @@ -586,7 +586,7 @@ func isTerminal(w io.Writer) bool { // applicationPackageFrom returns an application loaded from args. If args is empty, the application package is loaded // from the working directory. If requirePackaging is true, the application package is required to be packaged with mvn // package. -func (c *CLI) applicationPackageFrom(args []string, requirePackaging bool) (vespa.ApplicationPackage, error) { +func (c *CLI) applicationPackageFrom(args []string, options vespa.PackageOptions) (vespa.ApplicationPackage, error) { path := "." if len(args) == 1 { path = args[0] @@ -603,5 +603,5 @@ func (c *CLI) applicationPackageFrom(args []string, requirePackaging bool) (vesp } else if len(args) > 1 { return vespa.ApplicationPackage{}, fmt.Errorf("expected 0 or 1 arguments, got %d", len(args)) } - return vespa.FindApplicationPackage(path, requirePackaging) + return vespa.FindApplicationPackage(path, options) } diff --git a/client/go/internal/vespa/application.go b/client/go/internal/vespa/application.go index cb43578af32..6d28b24100f 100644 --- a/client/go/internal/vespa/application.go +++ b/client/go/internal/vespa/application.go @@ -234,6 +234,14 @@ func copyFile(src *zip.File, dst string) error { return err } +type PackageOptions struct { + // If true, a Maven-based Vespa application package is required to be compiled + Compiled bool + + // If true, only consider the source directores of the application package + SourceOnly bool +} + // FindApplicationPackage finds the path to an application package from the zip file or directory zipOrDir. If // requirePackaging is true, the application package is required to be packaged with mvn package. // @@ -242,8 +250,8 @@ func copyFile(src *zip.File, dst string) error { // 2. target/application // 3. src/main/application // 4. Given path, if it contains services.xml -func FindApplicationPackage(zipOrDir string, requirePackaging bool) (ApplicationPackage, error) { - pkg, err := findApplicationPackage(zipOrDir, requirePackaging) +func FindApplicationPackage(zipOrDir string, options PackageOptions) (ApplicationPackage, error) { + pkg, err := findApplicationPackage(zipOrDir, options) if err != nil { return ApplicationPackage{}, err } @@ -253,20 +261,20 @@ func FindApplicationPackage(zipOrDir string, requirePackaging bool) (Application return pkg, nil } -func findApplicationPackage(zipOrDir string, requirePackaging bool) (ApplicationPackage, error) { +func findApplicationPackage(zipOrDir string, options PackageOptions) (ApplicationPackage, error) { if isZip(zipOrDir) { return ApplicationPackage{Path: zipOrDir}, nil } // Pre-packaged application. We prefer the uncompressed application because this allows us to add // security/clients.pem to the package on-demand hasPOM := util.PathExists(filepath.Join(zipOrDir, "pom.xml")) - if hasPOM { + if hasPOM && !options.SourceOnly { path := filepath.Join(zipOrDir, "target", "application") if util.PathExists(path) { testPath := existingPath(filepath.Join(zipOrDir, "target", "application-test")) return ApplicationPackage{Path: path, TestPath: testPath}, nil } - if requirePackaging { + if options.Compiled { return ApplicationPackage{}, fmt.Errorf("found pom.xml, but %s does not exist: run 'mvn package' first", path) } } diff --git a/client/go/internal/vespa/deploy_test.go b/client/go/internal/vespa/deploy_test.go index d1dffe0f6d6..9dfdc47d8e6 100644 --- a/client/go/internal/vespa/deploy_test.go +++ b/client/go/internal/vespa/deploy_test.go @@ -146,9 +146,9 @@ func TestFindApplicationPackage(t *testing.T) { existingFiles: []string{filepath.Join(dir, "pom.xml"), filepath.Join(dir, "src/test/application/tests/foo.json")}, }) assertFindApplicationPackage(t, dir, pkgFixture{ - existingFile: filepath.Join(dir, "pom.xml"), - requirePackaging: true, - fail: true, + existingFile: filepath.Join(dir, "pom.xml"), + compiled: true, + fail: true, }) assertFindApplicationPackage(t, dir, pkgFixture{ expectedPath: filepath.Join(dir, "target", "application"), @@ -159,6 +159,12 @@ func TestFindApplicationPackage(t *testing.T) { expectedTestPath: filepath.Join(dir, "target", "application-test"), existingFiles: []string{filepath.Join(dir, "target", "application"), filepath.Join(dir, "target", "application-test")}, }) + assertFindApplicationPackage(t, dir, pkgFixture{ + expectedPath: filepath.Join(dir, "src", "main", "application"), + expectedTestPath: filepath.Join(dir, "src", "test", "application"), + existingFiles: []string{filepath.Join(dir, "target", "application"), filepath.Join(dir, "target", "application-test")}, + sourceOnly: true, + }) zip := filepath.Join(dir, "myapp.zip") assertFindApplicationPackage(t, zip, pkgFixture{ expectedPath: zip, @@ -195,7 +201,8 @@ type pkgFixture struct { expectedTestPath string existingFile string existingFiles []string - requirePackaging bool + compiled bool + sourceOnly bool fail bool } @@ -207,7 +214,7 @@ func assertFindApplicationPackage(t *testing.T, zipOrDir string, fixture pkgFixt for _, f := range fixture.existingFiles { writeFile(t, f) } - pkg, err := FindApplicationPackage(zipOrDir, fixture.requirePackaging) + pkg, err := FindApplicationPackage(zipOrDir, PackageOptions{Compiled: fixture.compiled, SourceOnly: fixture.sourceOnly}) assert.Equal(t, err != nil, fixture.fail, "Expected error for "+zipOrDir) assert.Equal(t, fixture.expectedPath, pkg.Path) assert.Equal(t, fixture.expectedTestPath, pkg.TestPath) diff --git a/client/go/internal/vespa/document/dispatcher.go b/client/go/internal/vespa/document/dispatcher.go index 153631a8f5e..786e7c332a4 100644 --- a/client/go/internal/vespa/document/dispatcher.go +++ b/client/go/internal/vespa/document/dispatcher.go @@ -156,7 +156,7 @@ func (d *Dispatcher) processResults() { defer d.wg.Done() for op := range d.results { d.statsMu.Lock() - d.stats.Add(op.result) + d.stats.Add(op.result, op.attempts > 1) d.statsMu.Unlock() retry := d.shouldRetry(op, op.result) d.logResult(op, retry) diff --git a/client/go/internal/vespa/document/http_test.go b/client/go/internal/vespa/document/http_test.go index 57c663d6f4c..b2c1139f95f 100644 --- a/client/go/internal/vespa/document/http_test.go +++ b/client/go/internal/vespa/document/http_test.go @@ -120,7 +120,7 @@ func TestClientSend(t *testing.T) { if !reflect.DeepEqual(res, wantRes) { t.Fatalf("#%d: got result %+v, want %+v", i, res, wantRes) } - stats.Add(res) + stats.Add(res, false) r := httpClient.LastRequest if r.Method != tt.method { t.Errorf("#%d: got r.Method = %q, want %q", i, r.Method, tt.method) @@ -139,8 +139,9 @@ func TestClientSend(t *testing.T) { } } want := Stats{ - Requests: 5, - Responses: 4, + Operations: 5, + Requests: 5, + Responses: 4, ResponsesByCode: map[int]int64{ 200: 3, 502: 1, diff --git a/client/go/internal/vespa/document/stats.go b/client/go/internal/vespa/document/stats.go index 82630b4af1c..9ce10f2f250 100644 --- a/client/go/internal/vespa/document/stats.go +++ b/client/go/internal/vespa/document/stats.go @@ -40,16 +40,29 @@ func (r Result) Success() bool { // Stats represents feeding operation statistics. type Stats struct { + // Number of operations passed to the feeder by the user, not counting retries. + Operations int64 + // Number of responses received, grouped by the HTTP status code. Requests that do not receive a response (i.e. no + // status code) are not counted. ResponsesByCode map[int]int64 - Requests int64 - Responses int64 - Errors int64 - Inflight int64 - TotalLatency time.Duration - MinLatency time.Duration - MaxLatency time.Duration - BytesSent int64 - BytesRecv int64 + // Number of requests made, including retries. + Requests int64 + // Number of responses received. + Responses int64 + // Number of transport layer errors. + Errors int64 + // Number of requests currently in-flight. + Inflight int64 + // Sum of response latency + TotalLatency time.Duration + // Lowest recorded response latency + MinLatency time.Duration + // Highest recorded response latency + MaxLatency time.Duration + // Total bytes sent + BytesSent int64 + // Total bytes received + BytesRecv int64 } // AvgLatency returns the average latency for a request. @@ -82,14 +95,16 @@ func (s Stats) Clone() Stats { } // Add statistics from result to this. -func (s *Stats) Add(result Result) { +func (s *Stats) Add(result Result, retry bool) { + if !retry { + s.Operations++ + } s.Requests++ if s.ResponsesByCode == nil { s.ResponsesByCode = make(map[int]int64) } if result.Err == nil { - responsesByCode := s.ResponsesByCode[result.HTTPStatus] - s.ResponsesByCode[result.HTTPStatus] = responsesByCode + 1 + s.ResponsesByCode[result.HTTPStatus]++ s.Responses++ } else { s.Errors++ diff --git a/client/go/internal/vespa/document/stats_test.go b/client/go/internal/vespa/document/stats_test.go index c9b80878b75..a08dc075850 100644 --- a/client/go/internal/vespa/document/stats_test.go +++ b/client/go/internal/vespa/document/stats_test.go @@ -9,17 +9,19 @@ import ( func TestStatsAdd(t *testing.T) { var stats Stats - stats.Add(Result{HTTPStatus: 200, Latency: 200 * time.Millisecond}) - stats.Add(Result{HTTPStatus: 200, Latency: 400 * time.Millisecond}) - stats.Add(Result{HTTPStatus: 200, Latency: 100 * time.Millisecond}) - stats.Add(Result{HTTPStatus: 200, Latency: 500 * time.Millisecond}) - stats.Add(Result{HTTPStatus: 200, Latency: 300 * time.Millisecond}) - stats.Add(Result{HTTPStatus: 500, Latency: 100 * time.Millisecond}) + stats.Add(Result{HTTPStatus: 200, Latency: 200 * time.Millisecond}, false) + stats.Add(Result{HTTPStatus: 200, Latency: 400 * time.Millisecond}, false) + stats.Add(Result{HTTPStatus: 200, Latency: 100 * time.Millisecond}, false) + stats.Add(Result{HTTPStatus: 200, Latency: 500 * time.Millisecond}, false) + stats.Add(Result{HTTPStatus: 200, Latency: 300 * time.Millisecond}, false) + stats.Add(Result{HTTPStatus: 500, Latency: 100 * time.Millisecond}, false) + stats.Add(Result{HTTPStatus: 200, Latency: 100 * time.Millisecond}, true) expected := Stats{ - Requests: 6, - Responses: 6, - ResponsesByCode: map[int]int64{200: 5, 500: 1}, - TotalLatency: 1600 * time.Millisecond, + Operations: 6, + Requests: 7, + Responses: 7, + ResponsesByCode: map[int]int64{200: 6, 500: 1}, + TotalLatency: 1700 * time.Millisecond, MinLatency: 100 * time.Millisecond, MaxLatency: 500 * time.Millisecond, } @@ -33,11 +35,11 @@ func TestStatsAdd(t *testing.T) { func TestStatsClone(t *testing.T) { var a Stats - a.Add(Result{HTTPStatus: 200}) + a.Add(Result{HTTPStatus: 200}, false) b := a.Clone() - a.Add(Result{HTTPStatus: 200}) + a.Add(Result{HTTPStatus: 200}, false) - want := Stats{Requests: 1, Responses: 1, ResponsesByCode: map[int]int64{200: 1}} + want := Stats{Operations: 1, Requests: 1, Responses: 1, ResponsesByCode: map[int]int64{200: 1}} if !reflect.DeepEqual(b, want) { t.Errorf("got %+v, want %+v", b, want) } diff --git a/client/go/internal/vespa/target.go b/client/go/internal/vespa/target.go index 8c3f5c9b7c3..f3a94f762ff 100644 --- a/client/go/internal/vespa/target.go +++ b/client/go/internal/vespa/target.go @@ -9,7 +9,6 @@ import ( "io" "net/http" "strings" - "sync" "time" "github.com/vespa-engine/vespa/client/go/internal/util" @@ -51,9 +50,9 @@ type Service struct { TLSOptions TLSOptions deployAPI bool - once sync.Once auth Authenticator httpClient util.HTTPClient + customClient bool retryInterval time.Duration } @@ -109,7 +108,10 @@ type LogOptions struct { // Do sends request to this service. Authentication of the request happens automatically. func (s *Service) Do(request *http.Request, timeout time.Duration) (*http.Response, error) { - util.ConfigureTLS(s.httpClient, s.TLSOptions.KeyPair, s.TLSOptions.CACertificate, s.TLSOptions.TrustAll) + if !s.customClient { + // Do not override TLS config if a custom client has been configured + util.ConfigureTLS(s.httpClient, s.TLSOptions.KeyPair, s.TLSOptions.CACertificate, s.TLSOptions.TrustAll) + } if s.auth != nil { if err := s.auth.Authenticate(request); err != nil { return nil, fmt.Errorf("%w: %s", errAuth, err) @@ -118,8 +120,11 @@ func (s *Service) Do(request *http.Request, timeout time.Duration) (*http.Respon return s.httpClient.Do(request, timeout) } -// SetClient sets the HTTP client that this service should use. -func (s *Service) SetClient(client util.HTTPClient) { s.httpClient = client } +// SetClient sets a custom HTTP client that this service should use. +func (s *Service) SetClient(client util.HTTPClient) { + s.httpClient = client + s.customClient = true +} // Wait polls the health check of this service until it succeeds or timeout passes. func (s *Service) Wait(timeout time.Duration) error { diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index 1b505cdbfae..78b32d8af7b 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -1276,19 +1276,18 @@ "public boolean useV8GeoPositions()", "public int maxCompactBuffers()", "public java.util.List ignoredHttpUserAgents()", - "public com.yahoo.config.provision.NodeResources$Architecture adminClusterArchitecture()", "public boolean enableProxyProtocolMixedMode()", "public java.lang.String logFileCompressionAlgorithm(java.lang.String)", "public boolean enableGlobalPhase()", "public java.lang.String summaryDecodePolicy()", "public boolean enableNestedMultivalueGrouping()", - "public boolean useReconfigurableDispatcher()", "public int contentLayerMetadataFeatureLevel()", "public boolean dynamicHeapSize()", "public java.lang.String unknownConfigDefinition()", "public int searchHandlerThreadpool()", "public long mergingMaxMemoryUsagePerNode()", - "public boolean usePerDocumentThrottledDeleteBucket()" + "public boolean usePerDocumentThrottledDeleteBucket()", + "public boolean alwaysMarkPhraseExpensive()" ], "fields" : [ ] }, @@ -1454,7 +1453,9 @@ "methods" : [ "public abstract long aggregatedModelCostInBytes()", "public abstract void registerModel(com.yahoo.config.application.api.ApplicationFile)", - "public abstract void registerModel(java.net.URI)" + "public abstract void registerModel(com.yahoo.config.application.api.ApplicationFile, com.yahoo.config.model.api.OnnxModelOptions)", + "public abstract void registerModel(java.net.URI)", + "public abstract void registerModel(java.net.URI, com.yahoo.config.model.api.OnnxModelOptions)" ], "fields" : [ ] }, @@ -1472,7 +1473,9 @@ "public com.yahoo.config.model.api.OnnxModelCost$Calculator newCalculator(com.yahoo.config.application.api.ApplicationPackage, com.yahoo.config.provision.ApplicationId)", "public long aggregatedModelCostInBytes()", "public void registerModel(com.yahoo.config.application.api.ApplicationFile)", - "public void registerModel(java.net.URI)" + "public void registerModel(com.yahoo.config.application.api.ApplicationFile, com.yahoo.config.model.api.OnnxModelOptions)", + "public void registerModel(java.net.URI)", + "public void registerModel(java.net.URI, com.yahoo.config.model.api.OnnxModelOptions)" ], "fields" : [ ] }, @@ -1490,6 +1493,51 @@ ], "fields" : [ ] }, + "com.yahoo.config.model.api.OnnxModelOptions$GpuDevice" : { + "superClass" : "java.lang.Record", + "interfaces" : [ ], + "attributes" : [ + "public", + "final", + "record" + ], + "methods" : [ + "public void <init>(int, boolean)", + "public void <init>(int)", + "public final java.lang.String toString()", + "public final int hashCode()", + "public final boolean equals(java.lang.Object)", + "public int deviceNumber()", + "public boolean required()" + ], + "fields" : [ ] + }, + "com.yahoo.config.model.api.OnnxModelOptions" : { + "superClass" : "java.lang.Record", + "interfaces" : [ ], + "attributes" : [ + "public", + "final", + "record" + ], + "methods" : [ + "public void <init>(java.lang.String, int, int, com.yahoo.config.model.api.OnnxModelOptions$GpuDevice)", + "public void <init>(java.util.Optional, java.util.Optional, java.util.Optional, java.util.Optional)", + "public static com.yahoo.config.model.api.OnnxModelOptions empty()", + "public com.yahoo.config.model.api.OnnxModelOptions withExecutionMode(java.lang.String)", + "public com.yahoo.config.model.api.OnnxModelOptions withInterOpThreads(java.lang.Integer)", + "public com.yahoo.config.model.api.OnnxModelOptions withIntraOpThreads(java.lang.Integer)", + "public com.yahoo.config.model.api.OnnxModelOptions withGpuDevice(com.yahoo.config.model.api.OnnxModelOptions$GpuDevice)", + "public final java.lang.String toString()", + "public final int hashCode()", + "public final boolean equals(java.lang.Object)", + "public java.util.Optional executionMode()", + "public java.util.Optional interOpThreads()", + "public java.util.Optional intraOpThreads()", + "public java.util.Optional gpuDevice()" + ], + "fields" : [ ] + }, "com.yahoo.config.model.api.PortInfo" : { "superClass" : "java.lang.Object", "interfaces" : [ ], diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java index 188c4a32978..6c9070c9d8a 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java @@ -27,8 +27,6 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; -import static com.yahoo.config.provision.NodeResources.Architecture; - /** * Model context containing state provided to model factories. * @@ -106,19 +104,18 @@ public interface ModelContext { @ModelFeatureFlag(owners = {"arnej"}) default boolean useV8GeoPositions() { return false; } @ModelFeatureFlag(owners = {"baldersheim", "geirst", "toregge"}) default int maxCompactBuffers() { return 1; } @ModelFeatureFlag(owners = {"arnej", "andreer"}) default List<String> ignoredHttpUserAgents() { return List.of(); } - @ModelFeatureFlag(owners = {"hmusum"}, removeAfter = "8.250") default Architecture adminClusterArchitecture() { return Architecture.getDefault(); } @ModelFeatureFlag(owners = {"tokle"}) default boolean enableProxyProtocolMixedMode() { return true; } @ModelFeatureFlag(owners = {"arnej"}) default String logFileCompressionAlgorithm(String defVal) { return defVal; } - @ModelFeatureFlag(owners = {"arnej, bjorncs"}) default boolean enableGlobalPhase() { return true; } + @ModelFeatureFlag(owners = {"arnej, bjorncs"}, removeAfter = "8.262") default boolean enableGlobalPhase() { return true; } @ModelFeatureFlag(owners = {"baldersheim"}, comment = "Select summary decode type") default String summaryDecodePolicy() { return "eager"; } - @ModelFeatureFlag(owners = {"baldersheim"}) default boolean enableNestedMultivalueGrouping() { return false; } - @ModelFeatureFlag(owners = {"jonmv"}, removeAfter = "8.250") default boolean useReconfigurableDispatcher() { return true; } + @ModelFeatureFlag(owners = {"baldersheim"}, removeAfter = "8.261") default boolean enableNestedMultivalueGrouping() { return true; } @ModelFeatureFlag(owners = {"vekterli"}) default int contentLayerMetadataFeatureLevel() { return 0; } @ModelFeatureFlag(owners = {"bjorncs"}) default boolean dynamicHeapSize() { return false; } @ModelFeatureFlag(owners = {"hmusum"}) default String unknownConfigDefinition() { return "warn"; } @ModelFeatureFlag(owners = {"hmusum"}) default int searchHandlerThreadpool() { return 2; } @ModelFeatureFlag(owners = {"vekterli"}) default long mergingMaxMemoryUsagePerNode() { return -1; } @ModelFeatureFlag(owners = {"vekterli"}) default boolean usePerDocumentThrottledDeleteBucket() { return false; } + @ModelFeatureFlag(owners = {"baldersheim"}) default boolean alwaysMarkPhraseExpensive() { return false; } } /** Warning: As elsewhere in this package, do not make backwards incompatible changes that will break old config models! */ diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/OnnxModelCost.java b/config-model-api/src/main/java/com/yahoo/config/model/api/OnnxModelCost.java index acb88070482..b98667457e4 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/OnnxModelCost.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/OnnxModelCost.java @@ -10,6 +10,7 @@ import java.net.URI; /** * @author bjorncs */ +// TODO: Rename public interface OnnxModelCost { Calculator newCalculator(ApplicationPackage appPkg, ApplicationId applicationId); @@ -17,7 +18,9 @@ public interface OnnxModelCost { interface Calculator { long aggregatedModelCostInBytes(); void registerModel(ApplicationFile path); + void registerModel(ApplicationFile path, OnnxModelOptions onnxModelOptions); void registerModel(URI uri); + void registerModel(URI uri, OnnxModelOptions onnxModelOptions); } static OnnxModelCost disabled() { return new DisabledOnnxModelCost(); } @@ -26,7 +29,9 @@ public interface OnnxModelCost { @Override public Calculator newCalculator(ApplicationPackage appPkg, ApplicationId applicationId) { return this; } @Override public long aggregatedModelCostInBytes() {return 0;} @Override public void registerModel(ApplicationFile path) {} + @Override public void registerModel(ApplicationFile path, OnnxModelOptions onnxModelOptions) {} @Override public void registerModel(URI uri) {} + @Override public void registerModel(URI uri, OnnxModelOptions onnxModelOptions) {} } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/OnnxModelOptions.java b/config-model-api/src/main/java/com/yahoo/config/model/api/OnnxModelOptions.java index 6347f0dc427..92817baae3f 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/OnnxModelOptions.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/OnnxModelOptions.java @@ -1,5 +1,5 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.model.container.component; +package com.yahoo.config.model.api; import java.util.Optional; @@ -12,7 +12,11 @@ import java.util.Optional; public record OnnxModelOptions(Optional<String> executionMode, Optional<Integer> interOpThreads, Optional<Integer> intraOpThreads, Optional<GpuDevice> gpuDevice) { - public static OnnxModelOptions empty() { + public OnnxModelOptions(String executionMode, int interOpThreads, int intraOpThreads, GpuDevice gpuDevice) { + this(Optional.of(executionMode), Optional.of(interOpThreads), Optional.of(intraOpThreads), Optional.of(gpuDevice)); + } + + public static OnnxModelOptions empty() { return new OnnxModelOptions(Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()); } diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java index f2444e1d222..41df042284e 100644 --- a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java +++ b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java @@ -129,7 +129,6 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea @Override public int maxCompactBuffers() { return maxCompactBuffers; } @Override public boolean useV8GeoPositions() { return useV8GeoPositions; } @Override public List<String> environmentVariables() { return environmentVariables; } - @Override public Architecture adminClusterArchitecture() { return adminClusterNodeResourcesArchitecture; } @Override public boolean sharedStringRepoNoReclaim() { return sharedStringRepoNoReclaim; } @Override public boolean loadCodeAsHugePages() { return loadCodeAsHugePages; } @Override public int mbusNetworkThreads() { return mbus_network_threads; } diff --git a/config-model/src/main/java/com/yahoo/schema/OnnxModel.java b/config-model/src/main/java/com/yahoo/schema/OnnxModel.java index 867ffdb3960..9456baafd57 100644 --- a/config-model/src/main/java/com/yahoo/schema/OnnxModel.java +++ b/config-model/src/main/java/com/yahoo/schema/OnnxModel.java @@ -1,9 +1,9 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.schema; +import com.yahoo.config.model.api.OnnxModelOptions; import com.yahoo.searchlib.rankingexpression.Reference; import com.yahoo.tensor.TensorType; -import com.yahoo.vespa.model.container.component.OnnxModelOptions; import com.yahoo.vespa.model.ml.OnnxModelInfo; import java.util.Collections; @@ -171,4 +171,6 @@ public class OnnxModel extends DistributableResource implements Cloneable { return onnxModelOptions.gpuDevice(); } + public OnnxModelOptions onnxModelOptions() { return onnxModelOptions; } + } diff --git a/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java b/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java index 8606599f530..6a1ebb39ac8 100644 --- a/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java +++ b/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java @@ -36,7 +36,6 @@ import java.util.ListIterator; import java.util.Map; import java.util.OptionalDouble; import java.util.Set; -import java.util.stream.Collectors; /** * A rank profile derived from a search definition, containing exactly the features available natively in the server @@ -171,7 +170,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer { private final OptionalDouble approximateThreshold; private final OptionalDouble targetHitsMaxAdjustmentFactor; private final double rankScoreDropLimit; - private final boolean enableNestedMultivalueGrouping; + private final boolean alwaysMarkPhraseExpensive; /** * The rank type definitions used to derive settings for the native rank features @@ -213,7 +212,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer { minHitsPerThread = compiled.getMinHitsPerThread(); numSearchPartitions = compiled.getNumSearchPartitions(); termwiseLimit = compiled.getTermwiseLimit().orElse(deployProperties.featureFlags().defaultTermwiseLimit()); - enableNestedMultivalueGrouping = deployProperties.featureFlags().enableNestedMultivalueGrouping(); + alwaysMarkPhraseExpensive = deployProperties.featureFlags().alwaysMarkPhraseExpensive(); postFilterThreshold = compiled.getPostFilterThreshold(); approximateThreshold = compiled.getApproximateThreshold(); targetHitsMaxAdjustmentFactor = compiled.getTargetHitsMaxAdjustmentFactor(); @@ -464,8 +463,8 @@ public class RawRankProfile implements RankProfilesConfig.Producer { if (termwiseLimit < 1.0) { properties.add(new Pair<>("vespa.matching.termwise_limit", termwiseLimit + "")); } - if (enableNestedMultivalueGrouping) { - properties.add(new Pair<>("vespa.temporary.enable_nested_multivalue_grouping", String.valueOf(enableNestedMultivalueGrouping))); + if (alwaysMarkPhraseExpensive) { + properties.add(new Pair<>("vespa.matching.always_mark_phrase_expensive", String.valueOf(alwaysMarkPhraseExpensive))); } if (postFilterThreshold.isPresent()) { properties.add(new Pair<>("vespa.matching.global_filter.upper_limit", String.valueOf(postFilterThreshold.getAsDouble()))); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/BertEmbedder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/BertEmbedder.java index ea3caadc23a..67fb720b8c0 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/BertEmbedder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/BertEmbedder.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.model.container.component; import com.yahoo.config.ModelReference; +import com.yahoo.config.model.api.OnnxModelOptions; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.embedding.BertBaseEmbedderConfig; import com.yahoo.vespa.model.container.ApplicationContainerCluster; @@ -47,7 +48,7 @@ public class BertEmbedder extends TypedComponent implements BertBaseEmbedderConf transformerStartSequenceToken = getChildValue(xml, "transformer-start-sequence-token").map(Integer::parseInt).orElse(null); transformerEndSequenceToken = getChildValue(xml, "transformer-end-sequence-token").map(Integer::parseInt).orElse(null); poolingStrategy = getChildValue(xml, "pooling-strategy").orElse(null); - model.registerOnnxModelCost(cluster); + model.registerOnnxModelCost(cluster, onnxModelOptions); } @Override diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/ColBertEmbedder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/ColBertEmbedder.java index cbae50b400c..d22e6afc3d1 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/ColBertEmbedder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/ColBertEmbedder.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.model.container.component; import com.yahoo.config.ModelReference; +import com.yahoo.config.model.api.OnnxModelOptions; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.embedding.ColBertEmbedderConfig; import com.yahoo.vespa.model.container.ApplicationContainerCluster; @@ -55,7 +56,7 @@ public class ColBertEmbedder extends TypedComponent implements ColBertEmbedderCo transformerInputIds = getChildValue(xml, "transformer-input-ids").orElse(null); transformerAttentionMask = getChildValue(xml, "transformer-attention-mask").orElse(null); transformerOutput = getChildValue(xml, "transformer-output").orElse(null); - model.registerOnnxModelCost(cluster); + model.registerOnnxModelCost(cluster, onnxModelOptions); } private static ModelReference resolveDefaultVocab(Model model, DeployState state) { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/HuggingFaceEmbedder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/HuggingFaceEmbedder.java index d1bd0dce000..d98c72ab3a4 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/HuggingFaceEmbedder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/HuggingFaceEmbedder.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.model.container.component; import com.yahoo.config.ModelReference; +import com.yahoo.config.model.api.OnnxModelOptions; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.embedding.huggingface.HuggingFaceEmbedderConfig; import com.yahoo.vespa.model.container.ApplicationContainerCluster; @@ -48,7 +49,7 @@ public class HuggingFaceEmbedder extends TypedComponent implements HuggingFaceEm transformerOutput = getChildValue(xml, "transformer-output").orElse(null); normalize = getChildValue(xml, "normalize").map(Boolean::parseBoolean).orElse(null); poolingStrategy = getChildValue(xml, "pooling-strategy").orElse(null); - model.registerOnnxModelCost(cluster); + model.registerOnnxModelCost(cluster, onnxModelOptions); } private static ModelReference resolveDefaultVocab(Model model, DeployState state) { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/Model.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/Model.java index c5daf23d6f8..0d350242fd0 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/Model.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/Model.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.model.container.component; import com.yahoo.config.ModelReference; import com.yahoo.config.application.api.ApplicationFile; +import com.yahoo.config.model.api.OnnxModelOptions; import com.yahoo.config.model.builder.xml.XmlHelper; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.path.Path; @@ -54,10 +55,10 @@ class Model { return new Model(ds, model.getTagName(), modelId, url, path); } - void registerOnnxModelCost(ApplicationContainerCluster c) { + void registerOnnxModelCost(ApplicationContainerCluster c, OnnxModelOptions onnxModelOptions) { var resolvedUrl = resolvedUrl().orElse(null); - if (file != null) c.onnxModelCost().registerModel(file); - else if (resolvedUrl != null) c.onnxModelCost().registerModel(resolvedUrl); + if (file != null) c.onnxModelCost().registerModel(file, onnxModelOptions); + else if (resolvedUrl != null) c.onnxModelCost().registerModel(resolvedUrl, onnxModelOptions); } String name() { return paramName; } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java index d86d117f1d2..6e4c14bdeac 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java @@ -51,16 +51,14 @@ public class ContainerSearch extends ContainerSubsystem<SearchChains> private final ApplicationContainerCluster owningCluster; private final List<SearchCluster> searchClusters = new LinkedList<>(); private final Collection<String> schemasWithGlobalPhase; - private final boolean globalPhase; + private final ApplicationPackage app; private QueryProfiles queryProfiles; private SemanticRules semanticRules; private PageTemplates pageTemplates; - private ApplicationPackage app; public ContainerSearch(DeployState deployState, ApplicationContainerCluster cluster, SearchChains chains) { super(chains); - this.globalPhase = deployState.featureFlags().enableGlobalPhase(); this.schemasWithGlobalPhase = getSchemasWithGlobalPhase(deployState); this.app = deployState.getApplicationPackage(); this.owningCluster = cluster; @@ -95,16 +93,14 @@ public class ContainerSearch extends ContainerSubsystem<SearchChains> var dispatcher = new DispatcherComponent(indexed, dispatcherClass); owningCluster.addComponent(dispatcher); } - if (globalPhase) { - for (var documentDb : searchCluster.getDocumentDbs()) { - if ( ! schemasWithGlobalPhase.contains(documentDb.getSchemaName())) continue; - var factory = new RankProfilesEvaluatorComponent(documentDb); - if ( ! owningCluster.getComponentsMap().containsKey(factory.getComponentId())) { - var onnxModels = documentDb.getDerivedConfiguration().getRankProfileList().getOnnxModels(); - onnxModels.asMap().forEach( - (__, model) -> owningCluster.onnxModelCost().registerModel(app.getFile(model.getFilePath()))); - owningCluster.addComponent(factory); - } + for (var documentDb : searchCluster.getDocumentDbs()) { + if ( ! schemasWithGlobalPhase.contains(documentDb.getSchemaName())) continue; + var factory = new RankProfilesEvaluatorComponent(documentDb); + if ( ! owningCluster.getComponentsMap().containsKey(factory.getComponentId())) { + var onnxModels = documentDb.getDerivedConfiguration().getRankProfileList().getOnnxModels(); + onnxModels.asMap().forEach( + (__, model) -> owningCluster.onnxModelCost().registerModel(app.getFile(model.getFilePath()), model.onnxModelOptions())); + owningCluster.addComponent(factory); } } } @@ -176,7 +172,6 @@ public class ContainerSearch extends ContainerSubsystem<SearchChains> } scB.rankprofiles(new QrSearchersConfig.Searchcluster.Rankprofiles.Builder().configid(sys.getConfigId())); scB.indexingmode(QrSearchersConfig.Searchcluster.Indexingmode.Enum.valueOf(sys.getIndexingModeName())); - scB.globalphase(globalPhase); if ( ! (sys instanceof IndexedSearchCluster)) { scB.storagecluster(new QrSearchersConfig.Searchcluster.Storagecluster.Builder(). routespec(((StreamingSearchCluster)sys).getStorageRouteSpec())); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 18020f5df5d..5ffd34c6557 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -800,7 +800,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { !container.getHostResource().realResources().gpuResources().isZero()); onnxModel.setGpuDevice(gpuDevice, hasGpu); } - cluster.onnxModelCost().registerModel(context.getApplicationPackage().getFile(onnxModel.getFilePath())); + cluster.onnxModelCost().registerModel(context.getApplicationPackage().getFile(onnxModel.getFilePath()), onnxModel.onnxModelOptions()); } cluster.setModelEvaluation(new ContainerModelEvaluation(cluster, profiles, models)); diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/JvmHeapSizeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/JvmHeapSizeValidatorTest.java index 8531aff3b1a..9cadf5cffd8 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/JvmHeapSizeValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/JvmHeapSizeValidatorTest.java @@ -1,14 +1,13 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - package com.yahoo.vespa.model.application.validation; import com.yahoo.config.application.api.ApplicationFile; import com.yahoo.config.application.api.ApplicationPackage; -import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.model.NullConfigModelRegistry; import com.yahoo.config.model.api.ApplicationClusterEndpoint; import com.yahoo.config.model.api.ContainerEndpoint; import com.yahoo.config.model.api.OnnxModelCost; +import com.yahoo.config.model.api.OnnxModelOptions; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.deploy.TestProperties; import com.yahoo.config.model.provision.InMemoryProvisioner; @@ -123,12 +122,20 @@ class JvmHeapSizeValidatorTest { @Override public Calculator newCalculator(ApplicationPackage appPkg, ApplicationId applicationId) { return this; } @Override public long aggregatedModelCostInBytes() { return totalCost.get(); } @Override public void registerModel(ApplicationFile path) {} + @Override public void registerModel(ApplicationFile path, OnnxModelOptions onnxModelOptions) {} @Override public void registerModel(URI uri) { assertEquals("https://my/url/model.onnx", uri.toString()); totalCost.addAndGet(modelCost); } + + @Override + public void registerModel(URI uri, OnnxModelOptions onnxModelOptions) { + assertEquals("https://my/url/model.onnx", uri.toString()); + totalCost.addAndGet(modelCost); + } + } } diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/NodeType.java b/config-provisioning/src/main/java/com/yahoo/config/provision/NodeType.java index 0eac568ec45..ff2cb26f250 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/NodeType.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/NodeType.java @@ -2,6 +2,7 @@ package com.yahoo.config.provision; import java.util.List; +import java.util.Optional; /** * The possible types of nodes in the node repository @@ -37,6 +38,13 @@ public enum NodeType { private final String description; private final List<NodeType> childNodeTypes; + public static Optional<NodeType> ofOptional(String name) { + for (var type : values()) { + if (type.name().equals(name)) return Optional.of(type); + } + return Optional.empty(); + } + NodeType(String description, NodeType... childNodeTypes) { this.childNodeTypes = List.of(childNodeTypes); this.description = description; diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java index 28d7c1eaef6..76af53eba90 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java @@ -45,7 +45,6 @@ import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.function.Predicate; -import static com.yahoo.config.provision.NodeResources.Architecture; import static com.yahoo.vespa.config.server.ConfigServerSpec.fromConfig; import static com.yahoo.vespa.flags.FetchVector.Dimension.CLUSTER_TYPE; @@ -190,7 +189,6 @@ public class ModelContextImpl implements ModelContext { private final boolean useV8GeoPositions; private final int maxCompactBuffers; private final List<String> ignoredHttpUserAgents; - private final Architecture adminClusterArchitecture; private final boolean enableProxyProtocolMixedMode; private final boolean sharedStringRepoNoReclaim; private final String logFileCompressionAlgorithm; @@ -202,9 +200,8 @@ public class ModelContextImpl implements ModelContext { private final int rpc_num_targets; private final int rpc_events_before_wakeup; private final int heapPercentage; - private final boolean enableGlobalPhase; private final String summaryDecodePolicy; - private final boolean enableNestedMultivalueGrouping; + private final boolean alwaysMarkPhraseExpensive; private final int contentLayerMetadataFeatureLevel; private final boolean dynamicHeapSize; private final String unknownConfigDefinition; @@ -235,7 +232,6 @@ public class ModelContextImpl implements ModelContext { this.useV8GeoPositions = flagValue(source, appId, version, Flags.USE_V8_GEO_POSITIONS); this.maxCompactBuffers = flagValue(source, appId, version, Flags.MAX_COMPACT_BUFFERS); this.ignoredHttpUserAgents = flagValue(source, appId, version, PermanentFlags.IGNORED_HTTP_USER_AGENTS); - this.adminClusterArchitecture = Architecture.valueOf(flagValue(source, appId, version, PermanentFlags.ADMIN_CLUSTER_NODE_ARCHITECTURE)); this.enableProxyProtocolMixedMode = flagValue(source, appId, version, Flags.ENABLE_PROXY_PROTOCOL_MIXED_MODE); this.sharedStringRepoNoReclaim = flagValue(source, appId, version, Flags.SHARED_STRING_REPO_NO_RECLAIM); this.logFileCompressionAlgorithm = flagValue(source, appId, version, Flags.LOG_FILE_COMPRESSION_ALGORITHM); @@ -248,15 +244,14 @@ public class ModelContextImpl implements ModelContext { this.queryDispatchPolicy = flagValue(source, appId, version, Flags.QUERY_DISPATCH_POLICY); this.queryDispatchWarmup = flagValue(source, appId, version, PermanentFlags.QUERY_DISPATCH_WARMUP); this.heapPercentage = flagValue(source, appId, version, PermanentFlags.HEAP_SIZE_PERCENTAGE); - this.enableGlobalPhase = flagValue(source, appId, version, Flags.ENABLE_GLOBAL_PHASE); this.summaryDecodePolicy = flagValue(source, appId, version, Flags.SUMMARY_DECODE_POLICY); - this.enableNestedMultivalueGrouping = flagValue(source, appId, version, Flags.ENABLE_NESTED_MULTIVALUE_GROUPING); this.contentLayerMetadataFeatureLevel = flagValue(source, appId, version, Flags.CONTENT_LAYER_METADATA_FEATURE_LEVEL); this.dynamicHeapSize = flagValue(source, appId, version, Flags.DYNAMIC_HEAP_SIZE); this.unknownConfigDefinition = flagValue(source, appId, version, Flags.UNKNOWN_CONFIG_DEFINITION); this.searchHandlerThreadpool = flagValue(source, appId, version, Flags.SEARCH_HANDLER_THREADPOOL); this.mergingMaxMemoryUsagePerNode = flagValue(source, appId, version, Flags.MERGING_MAX_MEMORY_USAGE_PER_NODE); this.usePerDocumentThrottledDeleteBucket = flagValue(source, appId, version, Flags.USE_PER_DOCUMENT_THROTTLED_DELETE_BUCKET); + this.alwaysMarkPhraseExpensive = flagValue(source, appId, version, Flags.ALWAYS_MARK_PHRASE_EXPENSIVE); } @Override public int heapSizePercentage() { return heapPercentage; } @@ -287,7 +282,6 @@ public class ModelContextImpl implements ModelContext { @Override public boolean useV8GeoPositions() { return useV8GeoPositions; } @Override public int maxCompactBuffers() { return maxCompactBuffers; } @Override public List<String> ignoredHttpUserAgents() { return ignoredHttpUserAgents; } - @Override public Architecture adminClusterArchitecture() { return adminClusterArchitecture; } @Override public boolean enableProxyProtocolMixedMode() { return enableProxyProtocolMixedMode; } @Override public boolean sharedStringRepoNoReclaim() { return sharedStringRepoNoReclaim; } @Override public int mbusJavaRpcNumTargets() { return mbus_java_num_targets; } @@ -303,8 +297,7 @@ public class ModelContextImpl implements ModelContext { } return defVal; } - @Override public boolean enableGlobalPhase() { return enableGlobalPhase; } - @Override public boolean enableNestedMultivalueGrouping() { return enableNestedMultivalueGrouping; } + @Override public boolean alwaysMarkPhraseExpensive() { return alwaysMarkPhraseExpensive; } @Override public int contentLayerMetadataFeatureLevel() { return contentLayerMetadataFeatureLevel; } @Override public boolean dynamicHeapSize() { return dynamicHeapSize; } @Override public String unknownConfigDefinition() { return unknownConfigDefinition; } diff --git a/container-core/abi-spec.json b/container-core/abi-spec.json index 3c11abb9d08..d07d21ae71f 100644 --- a/container-core/abi-spec.json +++ b/container-core/abi-spec.json @@ -660,6 +660,10 @@ "public com.yahoo.container.jdisc.HttpRequestBuilder withRequestContent(java.io.InputStream)", "public com.yahoo.container.jdisc.HttpRequestBuilder withScheme(java.lang.String)", "public com.yahoo.container.jdisc.HttpRequestBuilder withHostname(java.lang.String)", + "public com.yahoo.container.jdisc.HttpRequestBuilder withPrincipal(java.security.Principal)", + "public com.yahoo.container.jdisc.HttpRequestBuilder withRemoteAddress(java.net.SocketAddress)", + "public com.yahoo.container.jdisc.HttpRequestBuilder withAttribute(java.lang.String, java.lang.Object)", + "public com.yahoo.container.jdisc.HttpRequestBuilder withPort(int)", "public com.yahoo.container.jdisc.HttpRequest build()" ], "fields" : [ ] diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequestBuilder.java b/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequestBuilder.java index a2d792e6ae0..147f388e08c 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequestBuilder.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/HttpRequestBuilder.java @@ -4,6 +4,8 @@ package com.yahoo.container.jdisc; import com.yahoo.jdisc.http.HttpRequest.Method; import java.io.InputStream; +import java.net.SocketAddress; +import java.security.Principal; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -19,9 +21,13 @@ public class HttpRequestBuilder { private final String path; private final Map<String, List<String>> queryParameters = new TreeMap<>(); private final Map<String, String> headers = new TreeMap<>(); + private final Map<String, Object> attributes = new TreeMap<>(); private String scheme; private String hostname; private InputStream content; + private Principal principal; + private SocketAddress socketAddress; + private int port = -1; private HttpRequestBuilder(Method method, String path) { this.method = method; @@ -43,10 +49,20 @@ public class HttpRequestBuilder { public HttpRequestBuilder withHostname(String hostname) { this.hostname = hostname; return this; } + public HttpRequestBuilder withPrincipal(Principal p) { principal = p; return this; } + + public HttpRequestBuilder withRemoteAddress(SocketAddress sa) { socketAddress = sa; return this; } + + public HttpRequestBuilder withAttribute(String name, Object value) { attributes.put(name, value); return this; } + + public HttpRequestBuilder withPort(int port) { this.port = port; return this; } + public HttpRequest build() { String scheme = this.scheme != null ? this.scheme : "http"; String hostname = this.hostname != null ? this.hostname : "localhost"; - StringBuilder uriBuilder = new StringBuilder(scheme).append("://").append(hostname).append(path); + StringBuilder uriBuilder = new StringBuilder(scheme).append("://").append(hostname); + if (port > 0) uriBuilder.append(':').append(port); + uriBuilder.append(path); if (queryParameters.size() > 0) { uriBuilder.append('?'); queryParameters.forEach((name, values) -> { @@ -66,6 +82,9 @@ public class HttpRequestBuilder { request = HttpRequest.createTestRequest(uriBuilder.toString(), method); } headers.forEach((name, value) -> request.getJDiscRequest().headers().put(name, value)); + if (principal != null) request.getJDiscRequest().setUserPrincipal(principal); + if (socketAddress != null) request.getJDiscRequest().setRemoteAddress(socketAddress); + request.getJDiscRequest().context().putAll(attributes); return request; } } diff --git a/container-core/src/main/java/com/yahoo/restapi/RestApi.java b/container-core/src/main/java/com/yahoo/restapi/RestApi.java index 18d8d8c49b4..ee5628988c9 100644 --- a/container-core/src/main/java/com/yahoo/restapi/RestApi.java +++ b/container-core/src/main/java/com/yahoo/restapi/RestApi.java @@ -15,6 +15,7 @@ import com.yahoo.security.tls.ConnectionAuthContext; import javax.net.ssl.SSLSession; import java.io.InputStream; +import java.net.InetSocketAddress; import java.security.Principal; import java.util.List; import java.util.Optional; @@ -153,6 +154,7 @@ public interface RestApi { Principal userPrincipalOrThrow(); Optional<SSLSession> sslSession(); Optional<ConnectionAuthContext> connectionAuthContext(); + InetSocketAddress remoteAddress(); interface Parameters { Optional<String> getString(String name); @@ -193,6 +195,7 @@ public interface RestApi { interface FilterContext { RequestContext requestContext(); String route(); + void setPrincipal(Principal principal); HttpResponse executeNext(); } } diff --git a/container-core/src/main/java/com/yahoo/restapi/RestApiException.java b/container-core/src/main/java/com/yahoo/restapi/RestApiException.java index e3acf4258f1..3a44bc4da5f 100644 --- a/container-core/src/main/java/com/yahoo/restapi/RestApiException.java +++ b/container-core/src/main/java/com/yahoo/restapi/RestApiException.java @@ -57,6 +57,7 @@ public class RestApiException extends RuntimeException { } public static class BadRequest extends RestApiException { + public BadRequest() { this("Bad request"); } public BadRequest(String message) { this(message, null); } public BadRequest(Throwable cause) { this(cause.getMessage(), cause); } public BadRequest(String message, Throwable cause) { super(ErrorResponse::badRequest, message, cause); } @@ -69,6 +70,7 @@ public class RestApiException extends RuntimeException { } public static class Forbidden extends RestApiException { + public Forbidden() { this("Forbidden"); } public Forbidden(String message) { super(ErrorResponse::forbidden, message, null); } public Forbidden(String message, Throwable cause) { super(ErrorResponse::forbidden, message, cause); } } diff --git a/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java b/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java index 196a57d23bf..090e06c221f 100644 --- a/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java +++ b/container-core/src/main/java/com/yahoo/restapi/RestApiImpl.java @@ -21,6 +21,7 @@ import com.yahoo.security.tls.TransportSecurityUtils; import javax.net.ssl.SSLSession; import java.io.InputStream; +import java.net.InetSocketAddress; import java.net.URI; import java.security.Principal; import java.util.ArrayList; @@ -491,7 +492,7 @@ class RestApiImpl implements RestApi { @Override public Optional<ConnectionAuthContext> connectionAuthContext() { return sslSession().flatMap(TransportSecurityUtils::getConnectionAuthContext); } - + @Override public InetSocketAddress remoteAddress() { return (InetSocketAddress) request.getJDiscRequest().getRemoteAddress(); } private class PathParametersImpl implements RestApi.RequestContext.PathParameters { @Override @@ -558,6 +559,7 @@ class RestApiImpl implements RestApi { @Override public RestApi.RequestContext requestContext() { return requestContext; } @Override public String route() { return route.name != null ? route.name : route.pathPattern; } + @Override public void setPrincipal(Principal p) { requestContext.request.getJDiscRequest().setUserPrincipal(p); } HttpResponse executeFirst() { return filter.filterRequest(this); } diff --git a/container-core/src/main/resources/configdefinitions/container.qr-searchers.def b/container-core/src/main/resources/configdefinitions/container.qr-searchers.def index 96bd41483c6..7d5788c05cc 100644 --- a/container-core/src/main/resources/configdefinitions/container.qr-searchers.def +++ b/container-core/src/main/resources/configdefinitions/container.qr-searchers.def @@ -71,6 +71,3 @@ searchcluster[].indexingmode enum { REALTIME, STREAMING } default=REALTIME ## Storage cluster route to use for search cluster if indexingmode is streaming. searchcluster[].storagecluster.routespec string default="" - -## Enable global phase ranking -searchcluster[].globalphase bool default=false diff --git a/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java b/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java index 95ccff1a075..d27e04bbd7a 100644 --- a/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java +++ b/container-core/src/test/java/com/yahoo/restapi/RestApiImplTest.java @@ -182,6 +182,18 @@ class RestApiImplTest { "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Query parameter 'missing' is missing\"}"); } + @Test + void principal_from_filter_is_visible_to_handler() { + var restApi = RestApi.builder() + .addRoute(route("/api1").get(ctx -> ctx.userPrincipalOrThrow().getName())) + .addFilter(ctx -> { + ctx.setPrincipal(() -> "my-principal-name"); + return ctx.executeNext(); + }) + .build(); + verifyJsonResponse(restApi, Method.GET, "/api1", null, 200, "{\"message\":\"my-principal-name\"}"); + } + private static void verifyJsonResponse( RestApi restApi, Method method, String path, String requestContent, int expectedStatusCode, String expectedJson) { diff --git a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java index 12f8cdf9852..e20c4271fe0 100644 --- a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java +++ b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java @@ -83,7 +83,7 @@ public class ClusterSearcher extends Searcher { int searchClusterIndex = clusterConfig.clusterId(); searchClusterName = clusterConfig.clusterName(); QrSearchersConfig.Searchcluster searchClusterConfig = getSearchClusterConfigFromClusterName(qrsConfig, searchClusterName); - this.globalPhaseRanker = searchClusterConfig.globalphase() ? globalPhaseRanker : null; + this.globalPhaseRanker = globalPhaseRanker; schemas = new LinkedHashSet<>(); maxQueryTimeout = ParameterParser.asMilliSeconds(clusterConfig.maxQueryTimeout(), DEFAULT_MAX_QUERY_TIMEOUT); diff --git a/container-search/src/main/java/com/yahoo/vespa/streamingvisitors/StreamingSearcher.java b/container-search/src/main/java/com/yahoo/vespa/streamingvisitors/StreamingSearcher.java index 33f739e0cdb..3fb577a5ff3 100644 --- a/container-search/src/main/java/com/yahoo/vespa/streamingvisitors/StreamingSearcher.java +++ b/container-search/src/main/java/com/yahoo/vespa/streamingvisitors/StreamingSearcher.java @@ -136,10 +136,10 @@ public class StreamingSearcher extends VespaBackEndSearcher { initializeMissingQueryFields(query); if (documentSelectionQueryParameterCount(query) != 1) { - return new Result(query, ErrorMessage.createBackendCommunicationError("Streaming search needs one and " + - "only one of these query parameters to be set: " + - "streaming.userid, streaming.groupname, or " + - "streaming.selection")); + return new Result(query, ErrorMessage.createIllegalQuery("Streaming search needs one and " + + "only one of these query parameters to be set: " + + "streaming.userid, streaming.groupname, or " + + "streaming.selection")); } if (query.getTrace().isTraceable(4)) query.trace("Routing to search cluster " + getSearchClusterName() + " and document type " + documentType, 4); @@ -150,11 +150,11 @@ public class StreamingSearcher extends VespaBackEndSearcher { try { visitor.doSearch(); } catch (ParseException e) { - return new Result(query, ErrorMessage.createBackendCommunicationError("Failed to parse document selection string: " + - e.getMessage() + "'.")); + return new Result(query, ErrorMessage.createInvalidQueryParameter("Failed to parse document selection string: " + + e.getMessage())); } catch (TokenMgrException e) { - return new Result(query, ErrorMessage.createBackendCommunicationError("Failed to tokenize document selection string: " + - e.getMessage() + "'.")); + return new Result(query, ErrorMessage.createInvalidQueryParameter("Failed to tokenize document selection string: " + + e.getMessage())); } catch (TimeoutException e) { double elapsedMillis = durationInMillisFromNanoTime(timeStartedNanos); if ((effectiveTraceLevel > 0) && timeoutBadEnoughToBeReported(query, elapsedMillis)) { @@ -163,10 +163,9 @@ public class StreamingSearcher extends VespaBackEndSearcher { query, elapsedMillis / 1000.0))); } return new Result(query, ErrorMessage.createTimeout(e.getMessage())); - } catch (InterruptedException | IllegalArgumentException e) { + } catch (InterruptedException e) { return new Result(query, ErrorMessage.createBackendCommunicationError(e.getMessage())); } - return buildResultFromCompletedVisitor(query, visitor); } diff --git a/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/StreamingSearcherTestCase.java b/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/StreamingSearcherTestCase.java index e4d4d476429..dd5e1c71b16 100644 --- a/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/StreamingSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/vespa/streamingvisitors/StreamingSearcherTestCase.java @@ -244,17 +244,15 @@ public class StreamingSearcherTestCase { // Magic query values are used to trigger specific behaviors from mock visitor. checkError(searcher, "/?query=noselection", - "Backend communication error", "Streaming search needs one and only one"); + "Illegal query", "Streaming search needs one and only one"); checkError(searcher, "/?streaming.userid=1&query=parseexception", - "Backend communication error", "Failed to parse document selection string"); + "Invalid query parameter", "Failed to parse document selection string"); checkError(searcher, "/?streaming.userid=1&query=tokenizeexception", - "Backend communication error", "Failed to tokenize document selection string"); + "Invalid query parameter", "Failed to tokenize document selection string"); checkError(searcher, "/?streaming.userid=1&query=interruptedexception", "Backend communication error", "Interrupted"); checkError(searcher, "/?streaming.userid=1&query=timeoutexception", "Timed out", "Timed out"); - checkError(searcher, "/?streaming.userid=1&query=illegalargumentexception", - "Backend communication error", "Illegal argument"); checkError(searcher, "/?streaming.userid=1&query=nosummary", "Backend communication error", "Did not find summary for hit with document id"); checkError(searcher, "/?streaming.userid=1&query=nosummarytofill", diff --git a/dependency-versions/pom.xml b/dependency-versions/pom.xml index fc4ce0ff9f0..c4edf840c7c 100644 --- a/dependency-versions/pom.xml +++ b/dependency-versions/pom.xml @@ -66,7 +66,7 @@ <assertj.vespa.version>3.24.2</assertj.vespa.version> <!-- Athenz dependencies. Make sure these dependencies match those in Vespa's internal repositories --> - <athenz.vespa.version>1.11.46</athenz.vespa.version> + <athenz.vespa.version>1.11.47</athenz.vespa.version> <aws-sdk.vespa.version>1.12.590</aws-sdk.vespa.version> <!-- Athenz END --> @@ -76,8 +76,8 @@ find zkfacade/src/main/java/org/apache/curator -name package-info.java | \ xargs perl -pi -e 's/major = [0-9]+, minor = [0-9]+, micro = [0-9]+/major = 5, minor = 3, micro = 0/g' --> - <bouncycastle.vespa.version>1.77</bouncycastle.vespa.version> - <byte-buddy.vespa.version>1.14.9</byte-buddy.vespa.version> + <bouncycastle.vespa.version>1.76</bouncycastle.vespa.version> + <byte-buddy.vespa.version>1.14.10</byte-buddy.vespa.version> <checker-qual.vespa.version>3.38.0</checker-qual.vespa.version> <commons-beanutils.vespa.version>1.9.4</commons-beanutils.vespa.version> <commons-codec.vespa.version>1.16.0</commons-codec.vespa.version> @@ -162,7 +162,7 @@ <maven-plugin-api.vespa.version>${maven-core.vespa.version}</maven-plugin-api.vespa.version> <maven-plugin-tools.vespa.version>3.10.2</maven-plugin-tools.vespa.version> <maven-resources-plugin.vespa.version>3.3.1</maven-resources-plugin.vespa.version> - <maven-resolver.vespa.version>1.9.16</maven-resolver.vespa.version> + <maven-resolver.vespa.version>1.9.17</maven-resolver.vespa.version> <maven-shade-plugin.vespa.version>3.5.1</maven-shade-plugin.vespa.version> <maven-site-plugin.vespa.version>3.12.1</maven-site-plugin.vespa.version> <maven-source-plugin.vespa.version>3.3.0</maven-source-plugin.vespa.version> diff --git a/dist/vespa-engine.repo b/dist/vespa-engine.repo index de436eb16ad..d884b404533 100644 --- a/dist/vespa-engine.repo +++ b/dist/vespa-engine.repo @@ -1,3 +1,15 @@ +[vespa-open-source-rpms] +name=vespa-open-source-rpms +baseurl=https://dl.cloudsmith.io/public/vespa/open-source-rpms/rpm/el/8/$basearch +type=rpm-md +gpgkey=https://dl.cloudsmith.io/public/vespa/open-source-rpms/gpg.0F3DA3C70D35DA7B.key +gpgcheck=1 +repo_gpgcheck=1 +sslverify=1 +sslcacert=/etc/pki/tls/certs/ca-bundle.crt +pkg_gpgcheck=1 +enabled=1 + [copr:copr.fedorainfracloud.org:group_vespa:vespa] name=Copr repo for vespa owned by @vespa baseurl=https://download.copr.fedorainfracloud.org/results/@vespa/vespa/centos-stream-8-$basearch/ @@ -6,5 +18,5 @@ skip_if_unavailable=True gpgcheck=1 gpgkey=https://download.copr.fedorainfracloud.org/results/@vespa/vespa/pubkey.gpg repo_gpgcheck=0 -enabled=1 enabled_metadata=1 +enabled=0 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 7a7087fb434..f4096cc343f 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -295,13 +295,6 @@ public class Flags { "Takes effect on the next tick.", NODE_TYPE, HOSTNAME); - public static final UnboundBooleanFlag ENABLE_GLOBAL_PHASE = defineFeatureFlag( - "enable-global-phase", true, - List.of("arnej", "bjorncs"), "2023-02-28", "2024-01-10", - "Enable global phase ranking", - "Takes effect at redeployment", - INSTANCE_ID); - public static final UnboundBooleanFlag ENABLE_THE_ONE_THAT_SHOULD_NOT_BE_NAMED = defineFeatureFlag( "enable-the-one-that-should-not-be-named", false, List.of("hmusum"), "2023-05-08", "2023-12-01", "Whether to enable the one program that should not be named", @@ -313,10 +306,10 @@ public class Flags { "Where specified, CNAME records are used instead of the default ALIAS records, which have a default 60s TTL.", "Takes effect at redeployment from controller"); - public static final UnboundBooleanFlag ENABLE_NESTED_MULTIVALUE_GROUPING = defineFeatureFlag( - "enable-nested-multivalue-grouping", true, - List.of("baldersheim"), "2023-06-29", "2023-12-31", - "Should we enable proper nested multivalue grouping", + public static final UnboundBooleanFlag ALWAYS_MARK_PHRASE_EXPENSIVE = defineFeatureFlag( + "always-mark-phrase-expensive", false, + List.of("baldersheim"), "2023-11-20", "2023-12-31", + "If true all phrases will be marked expensive, independent of parents", "Takes effect at redeployment", INSTANCE_ID); diff --git a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java index 52cd8a8ff54..86d4e91a567 100644 --- a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java +++ b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java @@ -132,7 +132,7 @@ public class LinguisticsAnnotator { if (term != null) { addAnnotation(where, term, token.getOrig(), termOccurrences); if ( ! term.equals(lowercasedOrig)) - addAnnotation(where, token.getOrig(), token.getOrig(), termOccurrences); + addAnnotation(where, lowercasedOrig, token.getOrig(), termOccurrences); } for (int i = 0; i < token.getNumStems(); i++) { String stem = token.getStem(i); diff --git a/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java b/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java index a4dbe1fe826..0bdf98f2ae0 100644 --- a/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java +++ b/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java @@ -58,7 +58,9 @@ public class LinguisticsAnnotatorTestCase { @Test public void requireThatIndexableTokenStringsAreAnnotatedWithModeALL() { SpanTree expected = new SpanTree(SpanTrees.LINGUISTICS); - expected.spanList().span(0, 5).annotate(new Annotation(AnnotationTypes.TERM, new StringFieldValue("tesla"))); + var span1 = expected.spanList().span(0, 6); + span1.annotate(new Annotation(AnnotationTypes.TERM, new StringFieldValue("tesla"))); + span1.annotate(new Annotation(AnnotationTypes.TERM, new StringFieldValue("teslas"))); var span2 = expected.spanList().span(0, 4); span2.annotate(new Annotation(AnnotationTypes.TERM)); span2.annotate(new Annotation(AnnotationTypes.TERM, new StringFieldValue("car"))); @@ -69,7 +71,7 @@ public class LinguisticsAnnotatorTestCase { for (TokenType type : TokenType.values()) { if (!type.isIndexable()) continue; assertAnnotations(expected, "Tesla cars", new AnnotatorConfig().setStemMode("ALL"), - token("Tesla", "tesla", type), + token("Teslas", "tesla", type), token("cars", "car", type), SimpleToken.fromStems("ModelXes", List.of("modelxes", "modelx", "mex"))); } @@ -204,8 +206,7 @@ public class LinguisticsAnnotatorTestCase { @Test public void requireThatMaxTermOccurrencesIsHonored() { final String inputTerm = "foo"; - final String stemmedInputTerm = "bar"; // completely different from - // inputTerm for safer test + final String stemmedInputTerm = "bar"; // completely different from inputTerm for safer test final String paddedInputTerm = inputTerm + " "; final SpanTree expected = new SpanTree(SpanTrees.LINGUISTICS); final int inputTermOccurence = AnnotatorConfig.DEFAULT_MAX_TERM_OCCURRENCES * 2; diff --git a/jdisc-cloud-aws/src/main/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProvider.java b/jdisc-cloud-aws/src/main/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProvider.java index 6de8b5c0142..f307e903b19 100644 --- a/jdisc-cloud-aws/src/main/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProvider.java +++ b/jdisc-cloud-aws/src/main/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProvider.java @@ -43,7 +43,7 @@ public class VespaAwsCredentialsProvider implements AWSCredentialsProvider { @Override public AWSCredentials getCredentials() { Credentials sessionCredentials = credentials.get(); - if (Duration.between(clock.instant(), sessionCredentials.expiry).abs().compareTo(REFRESH_INTERVAL)<0) { + if (Duration.between(clock.instant(), sessionCredentials.expiry).compareTo(REFRESH_INTERVAL)<0) { refresh(); sessionCredentials = credentials.get(); } diff --git a/jdisc-cloud-aws/src/test/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProviderTest.java b/jdisc-cloud-aws/src/test/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProviderTest.java index 63cfd2f1eeb..889aa0f02c1 100644 --- a/jdisc-cloud-aws/src/test/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProviderTest.java +++ b/jdisc-cloud-aws/src/test/java/com/yahoo/jdisc/cloud/aws/VespaAwsCredentialsProviderTest.java @@ -27,6 +27,7 @@ public class VespaAwsCredentialsProviderTest { Instant updatedExpiry = clock.instant().plus(Duration.ofHours(24)); writeCredentials(credentialsPath, updatedExpiry); + // File updated, but old credentials still valid credentials = credentialsProvider.getCredentials(); assertExpiryEquals(originalExpiry, credentials); @@ -35,6 +36,13 @@ public class VespaAwsCredentialsProviderTest { clock.advance(Duration.ofHours(11).plus(Duration.ofMinutes(31))); credentials = credentialsProvider.getCredentials(); assertExpiryEquals(updatedExpiry, credentials); + + // Credentials refreshes when they are long expired (since noone asked for them for a long time) + updatedExpiry = clock.instant().plus(Duration.ofDays(12)); + writeCredentials(credentialsPath, updatedExpiry); + clock.advance(Duration.ofDays(11)); + credentials = credentialsProvider.getCredentials(); + assertExpiryEquals(updatedExpiry, credentials); } @Test diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java index 1fadc66c46a..47912e958c6 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java @@ -7,12 +7,15 @@ import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; +import java.io.IOException; + import static ai.vespa.metricsproxy.TestUtil.getFileContents; import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId; import static ai.vespa.metricsproxy.metric.model.MetricId.toMetricId; import static ai.vespa.metricsproxy.service.RemoteMetricsFetcher.METRICS_PATH; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author Unknown @@ -27,13 +30,9 @@ public class ContainerServiceTest { } @Before - public void setupHTTPServer() { - try { - String response = getFileContents("metrics-container-state-multi-chain.json"); - httpServer = new MockHttpServer(response, METRICS_PATH); - } catch (Exception e) { - e.printStackTrace(); - } + public void setupHTTPServer() throws IOException { + String response = getFileContents("metrics-container-state-multi-chain.json"); + httpServer = new MockHttpServer(response, METRICS_PATH); } @Test @@ -49,7 +48,7 @@ public class ContainerServiceTest { } else if (m.getDimensions().get(toDimensionId("chain")).equals("blendingResult")) { assertEquals(0.36666666666666664, m.getValue()); } else { - assertTrue("Unknown unknown chain", false); + fail("Unknown unknown chain"); } } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index a4a4c42d9c1..52b68708eee 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -263,8 +263,8 @@ public class NodeRepository extends AbstractComponent implements HealthCheckerPr loadBalancers.list(endpoint.applicationId()) .cluster(endpoint.clusterName()) .first() - .flatMap(LoadBalancer::instance) - .flatMap(LoadBalancerInstance::idSeed)); + .map(LoadBalancer::idSeed) + .orElseThrow(() -> new IllegalArgumentException("no load balancer for '" + endpoint + "'"))); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java index 36f2eb4bb2c..f8de6e1db16 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java @@ -17,12 +17,14 @@ import java.util.Set; public class LoadBalancer { private final LoadBalancerId id; + private final String idSeed; private final Optional<LoadBalancerInstance> instance; private final State state; private final Instant changedAt; - public LoadBalancer(LoadBalancerId id, Optional<LoadBalancerInstance> instance, State state, Instant changedAt) { + public LoadBalancer(LoadBalancerId id, String idSeed, Optional<LoadBalancerInstance> instance, State state, Instant changedAt) { this.id = Objects.requireNonNull(id, "id must be non-null"); + this.idSeed = Objects.requireNonNull(idSeed, "idSeed must be non-null"); this.instance = Objects.requireNonNull(instance, "instance must be non-null"); this.state = Objects.requireNonNull(state, "state must be non-null"); this.changedAt = Objects.requireNonNull(changedAt, "changedAt must be non-null"); @@ -40,6 +42,11 @@ public class LoadBalancer { return id; } + /** Seed to use for generating resource IDs for provisioned resources in this. */ + public String idSeed() { + return idSeed; + } + /** The instance associated with this */ public Optional<LoadBalancerInstance> instance() { return instance; @@ -64,12 +71,12 @@ public class LoadBalancer { if (this.state != State.reserved && state == State.reserved) { throw new IllegalArgumentException("Invalid state transition: " + this.state + " -> " + state); } - return new LoadBalancer(id, instance, state, changedAt); + return new LoadBalancer(id, idSeed, instance, state, changedAt); } /** Returns a copy of this with instance set to given instance */ public LoadBalancer with(LoadBalancerInstance instance) { - return new LoadBalancer(id, Optional.of(instance), state, changedAt); + return new LoadBalancer(id, idSeed, Optional.of(instance), state, changedAt); } public enum State { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java index 4dbf891b1b7..c0931ecbc70 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java @@ -20,7 +20,6 @@ import java.util.Set; */ public class LoadBalancerInstance { - private final Optional<String> idSeed; private final Optional<DomainName> hostname; private final Optional<String> ip4Address; private final Optional<String> ip6Address; @@ -32,10 +31,9 @@ public class LoadBalancerInstance { private final List<PrivateServiceId> serviceIds; private final CloudAccount cloudAccount; - public LoadBalancerInstance(Optional<String> idSeed, Optional<DomainName> hostname, Optional<String> ip4Address, Optional<String> ip6Address, + public LoadBalancerInstance(Optional<DomainName> hostname, Optional<String> ip4Address, Optional<String> ip6Address, Optional<DnsZone> dnsZone, Set<Integer> ports, Set<String> networks, Set<Real> reals, ZoneEndpoint settings, List<PrivateServiceId> serviceIds, CloudAccount cloudAccount) { - this.idSeed = Objects.requireNonNull(idSeed, "idSeed must be non-null"); this.hostname = Objects.requireNonNull(hostname, "hostname must be non-null"); this.ip4Address = Objects.requireNonNull(ip4Address, "ip4Address must be non-null"); this.ip6Address = Objects.requireNonNull(ip6Address, "ip6Address must be non-null"); @@ -53,11 +51,6 @@ public class LoadBalancerInstance { } } - /** A unique seed to use when generating cloud-specific resource IDs for this load balancer instance. */ - public Optional<String> idSeed() { - return idSeed; - } - /** Fully-qualified domain name of this load balancer. This hostname can be used for query and feed */ public Optional<DomainName> hostname() { return hostname; @@ -128,7 +121,7 @@ public class LoadBalancerInstance { public LoadBalancerInstance with(Set<Real> reals, ZoneEndpoint settings, Optional<PrivateServiceId> serviceId) { List<PrivateServiceId> ids = new ArrayList<>(serviceIds); serviceId.filter(id -> ! ids.contains(id)).ifPresent(ids::add); - return new LoadBalancerInstance(idSeed, hostname, ip4Address, ip6Address, dnsZone, ports, networks, + return new LoadBalancerInstance(hostname, ip4Address, ip6Address, dnsZone, ports, networks, reals, settings, ids, cloudAccount); } @@ -137,7 +130,7 @@ public class LoadBalancerInstance { public LoadBalancerInstance withServiceIds(List<PrivateServiceId> serviceIds) { List<PrivateServiceId> ids = new ArrayList<>(serviceIds); for (PrivateServiceId id : this.serviceIds) if ( ! ids.contains(id)) ids.add(id); - return new LoadBalancerInstance(idSeed, hostname, ip4Address, ip6Address, dnsZone, ports, networks, reals, settings, ids, cloudAccount); + return new LoadBalancerInstance(hostname, ip4Address, ip6Address, dnsZone, ports, networks, reals, settings, ids, cloudAccount); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java index 6ddde1151dd..efd1536d108 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java @@ -7,8 +7,6 @@ import com.yahoo.config.provision.EndpointsChecker.Endpoint; import com.yahoo.config.provision.EndpointsChecker.HealthChecker; import com.yahoo.config.provision.NodeType; -import java.util.Optional; - /** * A managed load balance service. * @@ -22,16 +20,7 @@ public interface LoadBalancerService { * @param spec Load balancer specification * @return The provisioned load balancer instance */ - default LoadBalancerInstance provision(LoadBalancerSpec spec) { return provision(spec, Optional.empty()); } - - /** - * Provisions load balancers from the given specification. Implementations are expected to be idempotent - * - * @param spec Load balancer specification - * @param idSeed Seed for generating a unique ID for the load balancer instance - * @return The provisioned load balancer instance - */ - LoadBalancerInstance provision(LoadBalancerSpec spec, Optional<String> idSeed); + LoadBalancerInstance provision(LoadBalancerSpec spec); /** * Configures load balancers for the given specification. Implementations are expected to be idempotent @@ -44,7 +33,7 @@ public interface LoadBalancerService { */ LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force); - void reallocate(LoadBalancerInstance provisioned, LoadBalancerSpec spec); + void reallocate(LoadBalancerSpec spec); /** Permanently remove given load balancer */ void remove(LoadBalancer loadBalancer); @@ -56,7 +45,7 @@ public interface LoadBalancerService { boolean supports(NodeType nodeType, ClusterSpec.Type clusterType); /** See {@link HealthChecker#healthy(Endpoint)}. */ - Availability healthy(Endpoint endpoint, Optional<String> idSeed); + Availability healthy(Endpoint endpoint, String idSeed); /** Load balancer protocols */ enum Protocol { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java index 03ff17c6ebc..2c672d0eada 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java @@ -26,19 +26,16 @@ import static java.util.stream.Collectors.toMap; public class LoadBalancerServiceMock implements LoadBalancerService { private record Key(ApplicationId application, ClusterSpec.Id cluster, String idSeed) { - @Override public int hashCode() { return idSeed == null ? Objects.hash(application, cluster) : Objects.hash(idSeed); } + @Override public int hashCode() { return idSeed.hashCode(); } @Override public boolean equals(Object o) { if (o == this) return true; if ( ! (o instanceof Key key)) return false; - if (idSeed != null) return Objects.equals(idSeed, key.idSeed); - return Objects.equals(application, key.application) && - Objects.equals(cluster, key.cluster); + return Objects.equals(idSeed, key.idSeed); } } private final Map<Key, LoadBalancerInstance> instances = new HashMap<>(); private boolean throwOnCreate = false; private boolean supportsProvisioning = true; - private final AtomicBoolean uuid = new AtomicBoolean(true); public Map<LoadBalancerId, LoadBalancerInstance> instances() { return instances.entrySet().stream().collect(toMap(e -> new LoadBalancerId(e.getKey().application, e.getKey().cluster), @@ -68,10 +65,9 @@ public class LoadBalancerServiceMock implements LoadBalancerService { } @Override - public LoadBalancerInstance provision(LoadBalancerSpec spec, Optional<String> idSeed) { + public LoadBalancerInstance provision(LoadBalancerSpec spec) { if (throwOnCreate) throw new IllegalStateException("Did not expect a new load balancer to be created"); var instance = new LoadBalancerInstance( - idSeed, Optional.of(DomainName.of("lb-" + spec.application().toShortString() + "-" + spec.cluster().value())), Optional.empty(), Optional.empty(), @@ -82,13 +78,13 @@ public class LoadBalancerServiceMock implements LoadBalancerService { spec.settings(), spec.settings().isPrivateEndpoint() ? List.of(PrivateServiceId.of("service")) : List.of(), spec.cloudAccount()); - instances.put(new Key(spec.application(), spec.cluster(), idSeed.orElse(null)), instance); + instances.put(new Key(spec.application(), spec.cluster(), spec.idSeed()), instance); return instance; } @Override public LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force) { - var id = new Key(spec.application(), spec.cluster(), instance.idSeed().orElse(null)); + var id = new Key(spec.application(), spec.cluster(), spec.idSeed()); var oldInstance = requireNonNull(instances.get(id), "expected existing load balancer " + id); if (!force && !oldInstance.reals().isEmpty() && spec.reals().isEmpty()) { throw new IllegalArgumentException("Refusing to remove all reals from load balancer " + id); @@ -101,21 +97,21 @@ public class LoadBalancerServiceMock implements LoadBalancerService { } @Override - public void reallocate(LoadBalancerInstance provisioned, LoadBalancerSpec spec) { - instances.put(new Key(spec.application(), spec.cluster(), provisioned.idSeed().get()), - requireNonNull(instances.remove(new Key(null, null, provisioned.idSeed().get())))); // ᕙ༼◕_◕༽ᕤ + public void reallocate(LoadBalancerSpec spec) { + instances.put(new Key(spec.application(), spec.cluster(), spec.idSeed()), + requireNonNull(instances.remove(new Key(null, null, spec.idSeed())))); // ᕙ༼◕_◕༽ᕤ } @Override public void remove(LoadBalancer loadBalancer) { requireNonNull(instances.remove(new Key(loadBalancer.id().application(), loadBalancer.id().cluster(), - loadBalancer.instance().get().idSeed().orElse(null))), + loadBalancer.idSeed())), "expected load balancer to exist: " + loadBalancer.id()); } @Override - public Availability healthy(Endpoint endpoint, Optional<String> idSeed) { + public Availability healthy(Endpoint endpoint, String idSeed) { return Availability.ready; } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java index bde736e7a28..8c0aee2b4c6 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerSpec.java @@ -16,20 +16,21 @@ import java.util.Set; * @author mpolden */ public record LoadBalancerSpec(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals, - ZoneEndpoint settings, CloudAccount cloudAccount) { + ZoneEndpoint settings, CloudAccount cloudAccount, String idSeed) { public static final ApplicationId preProvisionOwner = ApplicationId.from("hosted-vespa", "pre-provision", "default"); - public static LoadBalancerSpec preProvisionSpec(ClusterSpec.Id slot, CloudAccount account) { - return new LoadBalancerSpec(preProvisionOwner, slot, Set.of(), ZoneEndpoint.defaultEndpoint, account); + public static LoadBalancerSpec preProvisionSpec(ClusterSpec.Id slot, CloudAccount account, String idSeed) { + return new LoadBalancerSpec(preProvisionOwner, slot, Set.of(), ZoneEndpoint.defaultEndpoint, account, idSeed); } public LoadBalancerSpec(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals, - ZoneEndpoint settings, CloudAccount cloudAccount) { + ZoneEndpoint settings, CloudAccount cloudAccount, String idSeed) { this.application = Objects.requireNonNull(application); this.cluster = Objects.requireNonNull(cluster); this.reals = ImmutableSortedSet.copyOf(Objects.requireNonNull(reals)); this.settings = Objects.requireNonNull(settings); this.cloudAccount = Objects.requireNonNull(cloudAccount); + this.idSeed = Objects.requireNonNull(idSeed); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java index 883b8dec944..1ca7442adf5 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java @@ -30,25 +30,11 @@ public class SharedLoadBalancerService implements LoadBalancerService { } @Override - public LoadBalancerInstance provision(LoadBalancerSpec spec, Optional<String> idSeed) { - return create(spec); - } - - @Override - public LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force) { - return instance.with(spec.reals(), spec.settings(), Optional.empty()); - } - - @Override - public void reallocate(LoadBalancerInstance provisioned, LoadBalancerSpec spec) { - throw new UnsupportedOperationException("reallocate is not supported with " + getClass()); - } - - private LoadBalancerInstance create(LoadBalancerSpec spec) { + public LoadBalancerInstance provision(LoadBalancerSpec spec) { if ( ! spec.settings().isPublicEndpoint()) throw new IllegalArgumentException("non-public endpoints is not supported with " + getClass()); - return new LoadBalancerInstance(Optional.empty(), - Optional.of(DomainName.of(vipHostname)), + + return new LoadBalancerInstance(Optional.of(DomainName.of(vipHostname)), Optional.empty(), Optional.empty(), Optional.empty(), @@ -61,6 +47,16 @@ public class SharedLoadBalancerService implements LoadBalancerService { } @Override + public LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force) { + return instance.with(spec.reals(), spec.settings(), Optional.empty()); + } + + @Override + public void reallocate(LoadBalancerSpec spec) { + throw new UnsupportedOperationException("reallocate is not supported with " + getClass()); + } + + @Override public void remove(LoadBalancer loadBalancer) { // Do nothing, we have no external state to modify } @@ -78,7 +74,7 @@ public class SharedLoadBalancerService implements LoadBalancerService { } @Override - public Availability healthy(Endpoint endpoint, Optional<String> idSeed) { + public Availability healthy(Endpoint endpoint, String idSeed) { return Availability.ready; } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java index 04ea831c45c..105928c503e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java @@ -114,7 +114,8 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer { LOG.log(Level.INFO, () -> "Removing reals from inactive load balancer " + lb.id() + ": " + Sets.difference(lb.instance().get().reals(), reals)); LoadBalancerInstance instance = service.configure(lb.instance().get(), new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals, - lb.instance().get().settings(), lb.instance().get().cloudAccount()), + lb.instance().get().settings(), + lb.instance().get().cloudAccount(), lb.idSeed()), true); db.writeLoadBalancer(lb.with(instance), lb.state()); } catch (Exception e) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDb.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDb.java index d511570881b..31d79d34c94 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDb.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDb.java @@ -445,7 +445,7 @@ public class CuratorDb { } public Optional<LoadBalancer> readLoadBalancer(LoadBalancerId id) { - return read(loadBalancerPath(id), LoadBalancerSerializer::fromJson); + return read(loadBalancerPath(id), bytes -> LoadBalancerSerializer.fromJson(id, bytes)); } public void writeLoadBalancer(LoadBalancer loadBalancer, LoadBalancer.State fromState) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java index 756692917e3..99cc0d1e601 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java @@ -17,6 +17,7 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerInstance; import com.yahoo.vespa.hosted.provision.lb.PrivateServiceId; import com.yahoo.vespa.hosted.provision.lb.Real; +import com.yahoo.vespa.hosted.provision.provisioning.LoadBalancerProvisioner; import java.io.IOException; import java.io.UncheckedIOException; @@ -71,7 +72,7 @@ public class LoadBalancerSerializer { Cursor root = slime.setObject(); root.setString(idField, loadBalancer.id().serializedForm()); - loadBalancer.instance().flatMap(LoadBalancerInstance::idSeed).ifPresent(idSeed -> root.setString(idSeedField, idSeed)); + root.setString(idSeedField, loadBalancer.idSeed()); loadBalancer.instance().flatMap(LoadBalancerInstance::hostname).ifPresent(hostname -> root.setString(hostnameField, hostname.value())); loadBalancer.instance().flatMap(LoadBalancerInstance::ip4Address).ifPresent(ip -> root.setString(lbIpAddressField, ip)); loadBalancer.instance().flatMap(LoadBalancerInstance::ip6Address).ifPresent(ip -> root.setString(lbIp6AddressField, ip)); @@ -110,7 +111,7 @@ public class LoadBalancerSerializer { } } - public static LoadBalancer fromJson(byte[] data) { + public static LoadBalancer fromJson(LoadBalancerId id, byte[] data) { Cursor object = SlimeUtils.jsonToSlime(data).get(); Set<Real> reals = new LinkedHashSet<>(); @@ -127,7 +128,8 @@ public class LoadBalancerSerializer { Set<String> networks = new LinkedHashSet<>(); object.field(networksField).traverse((ArrayTraverser) (i, network) -> networks.add(network.asString())); - Optional<String> idSeed = SlimeUtils.optionalString(object.field(idSeedField)); + // TODO jonmv: remove fallback after data is re-written. + String idSeed = SlimeUtils.optionalString(object.field(idSeedField)).orElse(id.application().tenant().value() + id.application().application().value() + id.application().instance().value() + id.cluster().value()); Optional<DomainName> hostname = SlimeUtils.optionalString(object.field(hostnameField)).map(DomainName::of); Optional<String> ip4Address = SlimeUtils.optionalString(object.field(lbIpAddressField)); Optional<String> ip6Address = SlimeUtils.optionalString(object.field(lbIp6AddressField)); @@ -140,9 +142,10 @@ public class LoadBalancerSerializer { CloudAccount cloudAccount = optionalString(object.field(cloudAccountField), CloudAccount::from).orElse(CloudAccount.empty); Optional<LoadBalancerInstance> instance = hostname.isEmpty() && ip4Address.isEmpty() && ip6Address.isEmpty() ? Optional.empty() - : Optional.of(new LoadBalancerInstance(idSeed, hostname, ip4Address, ip6Address, dnsZone, ports, networks, reals, settings, serviceIds, cloudAccount)); + : Optional.of(new LoadBalancerInstance(hostname, ip4Address, ip6Address, dnsZone, ports, networks, reals, settings, serviceIds, cloudAccount)); return new LoadBalancer(LoadBalancerId.fromSerializedForm(object.field(idField).asString()), + idSeed, instance, stateFromString(object.field(stateField).asString()), Instant.ofEpochMilli(object.field(changedAtField).asLong())); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java index 24bca326f82..e6f2dc0fbfe 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java @@ -7,6 +7,7 @@ import com.yahoo.config.provision.ApplicationTransaction; import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.HostName; +import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.ZoneEndpoint; @@ -200,22 +201,21 @@ public class LoadBalancerProvisioner { } private void prepare(LoadBalancerId id, ZoneEndpoint zoneEndpoint, NodeSpec requested) { - Instant now = nodeRepository.clock().instant(); CloudAccount cloudAccount = requested.cloudAccount(); Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id); - LoadBalancer newLoadBalancer = loadBalancer.orElse(new LoadBalancer(id, Optional.empty(), LoadBalancer.State.reserved, now)); + LoadBalancer newLoadBalancer = null; LoadBalancer.State fromState = loadBalancer.map(LoadBalancer::state).orElse(null); try { if (loadBalancer.isPresent() && ! inAccount(cloudAccount, loadBalancer.get())) { - newLoadBalancer = newLoadBalancer.with(State.removable, now); + newLoadBalancer = loadBalancer.get().with(State.removable, nodeRepository.clock().instant()); throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in cloud account. The operation will be retried on next deployment"); } if (loadBalancer.isPresent() && ! hasCorrectVisibility(loadBalancer.get(), zoneEndpoint)) { - newLoadBalancer = newLoadBalancer.with(State.removable, now); + newLoadBalancer = loadBalancer.get().with(State.removable, nodeRepository.clock().instant()); throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in load balancer visibility. The operation will be retried on next deployment"); } - LoadBalancerInstance instance = provisionInstance(id, loadBalancer, zoneEndpoint, requested); - newLoadBalancer = newLoadBalancer.with(instance); + newLoadBalancer = loadBalancer.orElseGet(() -> createNewLoadBalancer(id, zoneEndpoint, requested)); // Determine id-seed. + newLoadBalancer = newLoadBalancer.with(provisionInstance(newLoadBalancer, zoneEndpoint, requested)); // Update instance. } catch (LoadBalancerServiceException e) { log.log(Level.WARNING, "Failed to provision load balancer", e); throw e; @@ -229,55 +229,43 @@ public class LoadBalancerProvisioner { newLoadBalancer.instance().get().settings().isPublicEndpoint() == zoneEndpoint.isPublicEndpoint(); } - private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, ZoneEndpoint settings, NodeList nodes) { - Instant now = nodeRepository.clock().instant(); - LoadBalancerId id = new LoadBalancerId(transaction.application(), cluster); - Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id); - if (loadBalancer.isEmpty()) throw new IllegalArgumentException("Could not activate load balancer that was never prepared: " + id); - if (loadBalancer.get().instance().isEmpty()) throw new IllegalArgumentException("Activating " + id + ", but prepare never provisioned a load balancer instance"); - - try { - LoadBalancerInstance instance = configureInstance(id, nodes, loadBalancer.get(), settings, loadBalancer.get().instance().get().cloudAccount()); - db.writeLoadBalancers(List.of(loadBalancer.get().with(instance).with(State.active, now)), - loadBalancer.get().state(), transaction.nested()); - } catch (LoadBalancerServiceException e) { - db.writeLoadBalancers(List.of(loadBalancer.get()), loadBalancer.get().state(), transaction.nested()); - throw e; - } + /** Creates a new load balancer, with an instance if one is taken from the pool, or without otherwise. */ + private LoadBalancer createNewLoadBalancer(LoadBalancerId id, ZoneEndpoint zoneEndpoint, NodeSpec requested) { + LoadBalancerSpec spec = new LoadBalancerSpec(id.application(), id.cluster(), Set.of(), zoneEndpoint, + requested.cloudAccount(), toSeed(id, requested.type())); + return provisionFromPool(spec, requested.type()) + .orElseGet(() -> new LoadBalancer(id, spec.idSeed(), Optional.empty(), State.reserved, nodeRepository.clock().instant())); } /** Provision a load balancer instance, if necessary */ - private LoadBalancerInstance provisionInstance(LoadBalancerId id, - Optional<LoadBalancer> currentLoadBalancer, + private LoadBalancerInstance provisionInstance(LoadBalancer currentLoadBalancer, ZoneEndpoint zoneEndpoint, NodeSpec requested) { - Set<Real> reals = currentLoadBalancer.flatMap(LoadBalancer::instance) + LoadBalancerId id = currentLoadBalancer.id(); + Set<Real> reals = currentLoadBalancer.instance() .map(LoadBalancerInstance::reals) .orElse(Set.of()); // Targeted reals are changed on activation. ZoneEndpoint settings = new ZoneEndpoint(zoneEndpoint.isPublicEndpoint(), zoneEndpoint.isPrivateEndpoint(), - currentLoadBalancer.flatMap(LoadBalancer::instance) + currentLoadBalancer.instance() .map(LoadBalancerInstance::settings) .map(ZoneEndpoint::allowedUrns) .orElse(List.of())); // Allowed URNs are changed on activation. - if ( currentLoadBalancer.isPresent() - && currentLoadBalancer.get().instance().isPresent() - && currentLoadBalancer.get().instance().get().settings().equals(settings)) - return currentLoadBalancer.get().instance().get(); + if (currentLoadBalancer.instance().map(instance -> settings.equals(instance.settings())).orElse(false)) + return currentLoadBalancer.instance().get(); log.log(Level.INFO, () -> "Provisioning instance for " + id); try { - LoadBalancerSpec spec = new LoadBalancerSpec(id.application(), id.cluster(), reals, settings, requested.cloudAccount()); - return provisionFromPool(spec, requested.type()).orElseGet(() -> service.provision(spec)) - // Provisioning a private endpoint service requires hard resources to be ready, so we delay it until activation. - .withServiceIds(currentLoadBalancer.flatMap(LoadBalancer::instance).map(LoadBalancerInstance::serviceIds).orElse(List.of())); + return service.provision(new LoadBalancerSpec(id.application(), id.cluster(), reals, settings, requested.cloudAccount(), currentLoadBalancer.idSeed())) + // Provisioning a private endpoint service requires hard resources to be ready, so we delay it until activation. + .withServiceIds(currentLoadBalancer.instance().map(LoadBalancerInstance::serviceIds).orElse(List.of())); } catch (Exception e) { throw new LoadBalancerServiceException("Could not provision " + id + ". The operation will be retried on next deployment.", e); } } - private Optional<LoadBalancerInstance> provisionFromPool(LoadBalancerSpec spec, NodeType type) { + private Optional<LoadBalancer> provisionFromPool(LoadBalancerSpec spec, NodeType type) { if (type != NodeType.tenant) return Optional.empty(); if ( ! spec.settings().isDefault()) return Optional.empty(); if (preProvisionPoolSize.value() == 0) return Optional.empty(); @@ -293,9 +281,14 @@ public class LoadBalancerProvisioner { if (chosen.state() != State.active || chosen.instance().isEmpty()) throw new IllegalStateException("expected active load balancer in pre-provisioned pool, but got " + chosen); log.log(Level.INFO, "Using " + chosen + " from pre-provisioned pool"); - service.reallocate(chosen.instance().get(), spec); + service.reallocate(new LoadBalancerSpec(spec.application(), spec.cluster(), spec.reals(), spec.settings(), spec.cloudAccount(), chosen.idSeed())); db.removeLoadBalancer(chosen.id()); // Using a transaction to remove this, and write the instance, would be better, but much hassle. - return chosen.instance(); // Should be immediately written again outside of this! + // Should be immediately written again outside of this! + return Optional.of(new LoadBalancer(new LoadBalancerId(spec.application(), spec.cluster()), + chosen.idSeed(), + chosen.instance(), + State.reserved, + nodeRepository.clock().instant())); } catch (Exception e) { log.log(Level.WARNING, "Failed to provision load balancer from pool", e); @@ -327,9 +320,11 @@ public class LoadBalancerProvisioner { // No need for lock while we provision, since we'll write atomically only after we're done, and the job lock ensures single writer. while (head - tail < size) { ClusterSpec.Id slot = slotId(head); - LoadBalancerSpec spec = preProvisionSpec(slot, nodeRepository.zone().cloud().account()); - db.writeLoadBalancer(new LoadBalancer(new LoadBalancerId(preProvisionOwner, slot), - Optional.of(service.provision(spec, Optional.of(slot.value()))), + LoadBalancerId id = new LoadBalancerId(preProvisionOwner, slot); + LoadBalancerSpec spec = preProvisionSpec(slot, nodeRepository.zone().cloud().account(), toSeed(id)); + db.writeLoadBalancer(new LoadBalancer(id, + spec.idSeed(), + Optional.of(service.provision(spec)), State.active, // Keep the expirer away. nodeRepository.clock().instant()), null); @@ -337,6 +332,38 @@ public class LoadBalancerProvisioner { } } + public static String toSeed(LoadBalancerId id, NodeType type) { + return type == NodeType.tenant ? toSeed(id) : toLegacySeed(id.application(), id.cluster()); + } + + public static String toSeed(LoadBalancerId id) { + return ":" + id.serializedForm() + ":"; // ಠ_ಠ+ } + + public static String toLegacySeed(ApplicationId application, ClusterSpec.Id cluster) { + return application.tenant().value() + + application.application().value() + + application.instance().value() + + cluster.value(); // ಠ_ಠ+ } + + private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, ZoneEndpoint settings, NodeList nodes) { + Instant now = nodeRepository.clock().instant(); + LoadBalancerId id = new LoadBalancerId(transaction.application(), cluster); + Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id); + if (loadBalancer.isEmpty()) throw new IllegalArgumentException("Could not activate load balancer that was never prepared: " + id); + if (loadBalancer.get().instance().isEmpty()) throw new IllegalArgumentException("Activating " + id + ", but prepare never provisioned a load balancer instance"); + + try { + LoadBalancerInstance instance = configureInstance(id, nodes, loadBalancer.get(), settings, loadBalancer.get().instance().get().cloudAccount()); + db.writeLoadBalancers(List.of(loadBalancer.get().with(instance).with(State.active, now)), + loadBalancer.get().state(), transaction.nested()); + } catch (LoadBalancerServiceException e) { + db.writeLoadBalancers(List.of(loadBalancer.get()), loadBalancer.get().state(), transaction.nested()); + throw e; + } + } + /** Reconfigure a load balancer instance, if necessary */ private LoadBalancerInstance configureInstance(LoadBalancerId id, NodeList nodes, LoadBalancer currentLoadBalancer, @@ -349,7 +376,7 @@ public class LoadBalancerProvisioner { log.log(Level.FINE, () -> "Configuring instance for " + id + ", targeting: " + reals); try { return service.configure(currentLoadBalancer.instance().orElseThrow(() -> new IllegalArgumentException("expected existing instance for " + id)), - new LoadBalancerSpec(id.application(), id.cluster(), reals, zoneEndpoint, cloudAccount), + new LoadBalancerSpec(id.application(), id.cluster(), reals, zoneEndpoint, cloudAccount, currentLoadBalancer.idSeed()), shouldDeactivateRouting || currentLoadBalancer.state() != LoadBalancer.State.active); } catch (Exception e) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionServiceProvider.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionServiceProvider.java index 20202ca7a74..6aafea637aa 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionServiceProvider.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionServiceProvider.java @@ -25,7 +25,7 @@ public interface ProvisionServiceProvider { } interface ProtoHealthChecker { - Availability healthy(Endpoint endpoint, Optional<String> idSeed); + Availability healthy(Endpoint endpoint, String idSeed); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java index f1a10134c7a..d47f5a4e4d6 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java @@ -50,6 +50,7 @@ public class LoadBalancersResponse extends SlimeJsonResponse { loadBalancers.forEach(lb -> { Cursor lbObject = loadBalancerArray.addObject(); lbObject.setString("id", lb.id().serializedForm()); + lbObject.setString("idSeed", lb.idSeed()); lbObject.setString("state", lb.state().name()); lbObject.setLong("changedAt", lb.changedAt().toEpochMilli()); lbObject.setString("application", lb.id().application().application().value()); @@ -90,7 +91,6 @@ public class LoadBalancersResponse extends SlimeJsonResponse { } instance.serviceId().ifPresent(serviceId -> lbObject.setString("serviceId", serviceId.value())); lbObject.setBool("public", instance.settings().isPublicEndpoint()); - instance.idSeed().ifPresent(idSeed -> lbObject.setString("idSeed", idSeed)); }); lb.instance() .map(LoadBalancerInstance::cloudAccount) diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java index 7475a3654b7..198709ba4bb 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java @@ -29,7 +29,7 @@ public class SharedLoadBalancerServiceTest { @Test public void test_create_lb() { LoadBalancerSpec spec = new LoadBalancerSpec(applicationId, clusterId, reals, - ZoneEndpoint.defaultEndpoint, CloudAccount.empty); + ZoneEndpoint.defaultEndpoint, CloudAccount.empty, "seed"); var lb = loadBalancerService.configure(loadBalancerService.provision(spec), spec, false); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java index 0d3c1994ad8..9ec63933921 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java @@ -38,8 +38,8 @@ public class LoadBalancerSerializerTest { var now = Instant.now(); { var loadBalancer = new LoadBalancer(loadBalancerId, + "1", Optional.of(new LoadBalancerInstance( - Optional.of("1"), Optional.of(DomainName.of("lb-host")), Optional.empty(), Optional.empty(), @@ -58,9 +58,9 @@ public class LoadBalancerSerializerTest { LoadBalancer.State.active, now); - var serialized = LoadBalancerSerializer.fromJson(LoadBalancerSerializer.toJson(loadBalancer)); + var serialized = LoadBalancerSerializer.fromJson(loadBalancer.id(), LoadBalancerSerializer.toJson(loadBalancer)); assertEquals(loadBalancer.id(), serialized.id()); - assertEquals(loadBalancer.instance().get().idSeed(), serialized.instance().get().idSeed()); + assertEquals(loadBalancer.idSeed(), serialized.idSeed()); assertEquals(loadBalancer.instance().get().hostname(), serialized.instance().get().hostname()); assertEquals(loadBalancer.instance().get().dnsZone(), serialized.instance().get().dnsZone()); assertEquals(loadBalancer.instance().get().ports(), serialized.instance().get().ports()); @@ -74,9 +74,9 @@ public class LoadBalancerSerializerTest { } { var loadBalancer = new LoadBalancer(loadBalancerId, + "", Optional.of(new LoadBalancerInstance( Optional.empty(), - Optional.empty(), Optional.of("1.2.3.4"), Optional.of("fd00::1"), Optional.of(new DnsZone("zone-id-1")), @@ -89,9 +89,9 @@ public class LoadBalancerSerializerTest { LoadBalancer.State.active, now); - var serialized = LoadBalancerSerializer.fromJson(LoadBalancerSerializer.toJson(loadBalancer)); + var serialized = LoadBalancerSerializer.fromJson(loadBalancer.id(), LoadBalancerSerializer.toJson(loadBalancer)); assertEquals(loadBalancer.id(), serialized.id()); - assertEquals(loadBalancer.instance().get().idSeed(), serialized.instance().get().idSeed()); + assertEquals(loadBalancer.idSeed(), serialized.idSeed()); assertEquals(loadBalancer.instance().get().hostname(), serialized.instance().get().hostname()); assertEquals(loadBalancer.instance().get().ip4Address(), serialized.instance().get().ip4Address()); assertEquals(loadBalancer.instance().get().ip6Address(), serialized.instance().get().ip6Address()); @@ -110,10 +110,11 @@ public class LoadBalancerSerializerTest { @Test public void no_instance_serialization() { var now = Instant.now(); - var loadBalancer = new LoadBalancer(loadBalancerId, Optional.empty(), LoadBalancer.State.reserved, now); + var loadBalancer = new LoadBalancer(loadBalancerId, "seed", Optional.empty(), LoadBalancer.State.reserved, now); - var serialized = LoadBalancerSerializer.fromJson(LoadBalancerSerializer.toJson(loadBalancer)); + var serialized = LoadBalancerSerializer.fromJson(loadBalancerId, LoadBalancerSerializer.toJson(loadBalancer)); assertEquals(loadBalancer.id(), serialized.id()); + assertEquals(loadBalancer.idSeed(), serialized.idSeed()); assertEquals(loadBalancer.instance(), serialized.instance()); assertEquals(loadBalancer.state(), serialized.state()); assertEquals(loadBalancer.changedAt().truncatedTo(MILLIS), serialized.changedAt()); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java index fb59b3077f8..7096ea3de46 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java @@ -50,6 +50,7 @@ import java.util.SortedSet; import java.util.function.Supplier; import java.util.stream.Collectors; +import static com.yahoo.vespa.hosted.provision.lb.LoadBalancerSpec.preProvisionOwner; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertSame; @@ -190,15 +191,15 @@ public class LoadBalancerProvisionerTest { provisioner.refreshPool(); expirer.run(); assertEquals(2, tester.nodeRepository().loadBalancers().list().size()); - assertEquals(2, tester.nodeRepository().loadBalancers().list(LoadBalancerSpec.preProvisionOwner).size()); + assertEquals(2, tester.nodeRepository().loadBalancers().list(preProvisionOwner).size()); // Provision a load balancer when the pool has two entries. ClusterSpec.Id containerCluster = ClusterSpec.Id.from("qrs"); prepare(app1, clusterRequest(ClusterSpec.Type.container, containerCluster)); List<LoadBalancer> loadBalancers = tester.nodeRepository().loadBalancers().list(app1).asList(); assertEquals(1, loadBalancers.size()); - assertEquals(1, tester.nodeRepository().loadBalancers().list(LoadBalancerSpec.preProvisionOwner).asList().size()); - assertEquals(Optional.of("1"), loadBalancers.get(0).instance().get().idSeed()); + assertEquals(1, tester.nodeRepository().loadBalancers().list(preProvisionOwner).asList().size()); + assertEquals(":" + preProvisionOwner.serializedForm() + ":1:", loadBalancers.get(0).idSeed()); // Shrink pool to 0 entries. flagSource.withIntFlag(PermanentFlags.PRE_PROVISIONED_LB_COUNT.id(), 0); @@ -214,9 +215,9 @@ public class LoadBalancerProvisionerTest { assertThrows(IllegalStateException.class, provisioner::refreshPool).getMessage()); tester.loadBalancerService().throwOnCreate(false); provisioner.refreshPool(); - assertEquals(List.of(Optional.of("3")), - tester.nodeRepository().loadBalancers().list(LoadBalancerSpec.preProvisionOwner) - .mapToList(lb -> lb.instance().get().idSeed())); + assertEquals(List.of(":" + preProvisionOwner.serializedForm() + ":3:"), + tester.nodeRepository().loadBalancers().list(preProvisionOwner) + .mapToList(LoadBalancer::idSeed)); } @Test diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers-single.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers-single.json index 620275bb033..ee8a8588c5f 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers-single.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers-single.json @@ -7,7 +7,7 @@ "dnsZone": "zone-id-1", "hostname": "lb-hosted-vespa.pre-provision-1", "id": "tenant4:application4:instance4:id4", - "idSeed": "1", + "idSeed": ":hosted-vespa:pre-provision:default:1:", "instance": "instance4", "networks": [ "10.2.3.0/24", diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers.json index e8392d92522..6de3b500a00 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/load-balancers.json @@ -7,6 +7,7 @@ "dnsZone": "zone-id-1", "hostname": "lb-tenant1.application1.instance1-id1", "id": "tenant1:application1:instance1:id1", + "idSeed": ":tenant1:application1:instance1:id1:", "instance": "instance1", "networks": [ "10.2.3.0/24", @@ -47,6 +48,7 @@ "dnsZone": "zone-id-1", "hostname": "lb-hosted-vespa.zone-config-servers-zone-config-servers", "id": "hosted-vespa:zone-config-servers:default:zone-config-servers", + "idSeed": "hosted-vespazone-config-serversdefaultzone-config-servers", "instance": "default", "networks": [ "10.2.3.0/24", @@ -78,7 +80,7 @@ "dnsZone": "zone-id-1", "hostname": "lb-hosted-vespa.pre-provision-1", "id": "tenant4:application4:instance4:id4", - "idSeed": "1", + "idSeed": ":hosted-vespa:pre-provision:default:1:", "instance": "instance4", "networks": [ "10.2.3.0/24", diff --git a/screwdriver.yaml b/screwdriver.yaml index c6707e2957f..446a0105556 100644 --- a/screwdriver.yaml +++ b/screwdriver.yaml @@ -516,6 +516,7 @@ jobs: screwdriver.cd/buildPeriodically: H 0 * * * steps: - install: | + dnf list llvm-libs --showduplicates dnf install -y dnf-plugins-core dnf config-manager --add-repo https://raw.githubusercontent.com/vespa-engine/vespa/master/dist/vespa-engine.repo dnf config-manager --enable powertools @@ -523,6 +524,7 @@ jobs: dnf install -y vespa mirror-copr-rpms-to-archive: + requires: [publish-release] image: docker.io/almalinux:8 annotations: screwdriver.cd/cpu: LOW diff --git a/searchcore/src/tests/grouping/grouping_test.cpp b/searchcore/src/tests/grouping/grouping_test.cpp index a2f646cba3a..fb46fb65f3b 100644 --- a/searchcore/src/tests/grouping/grouping_test.cpp +++ b/searchcore/src/tests/grouping/grouping_test.cpp @@ -45,7 +45,7 @@ struct MyWorld { bv.setInterval(0, NUM_DOCS); // attribute context { - SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr0"); + auto *attr = new SingleInt32ExtAttribute("attr0"); AttributeVector::DocId docid; for (uint32_t i = 0; i < NUM_DOCS; ++i) { attr->addDoc(docid); @@ -55,7 +55,7 @@ struct MyWorld { attributeContext.add(attr); } { - SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr1"); + auto *attr = new SingleInt32ExtAttribute("attr1"); AttributeVector::DocId docid; for (uint32_t i = 0; i < NUM_DOCS; ++i) { attr->addDoc(docid); @@ -65,7 +65,7 @@ struct MyWorld { attributeContext.add(attr); } { - SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr2"); + auto *attr = new SingleInt32ExtAttribute("attr2"); AttributeVector::DocId docid; for (uint32_t i = 0; i < NUM_DOCS; ++i) { attr->addDoc(docid); @@ -75,7 +75,7 @@ struct MyWorld { attributeContext.add(attr); } { - SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr3"); + auto *attr = new SingleInt32ExtAttribute("attr3"); AttributeVector::DocId docid; for (uint32_t i = 0; i < NUM_DOCS; ++i) { attr->addDoc(docid); @@ -94,16 +94,17 @@ using GroupingList = GroupingContext::GroupingList; SessionId createSessionId(const std::string & s) { std::vector<char> vec; - for (size_t i = 0; i < s.size(); i++) { - vec.push_back(s[i]); + for (char c : s) { + vec.push_back(c); } - return SessionId(&vec[0], vec.size()); + return {vec.data(), vec.size()}; } class CheckAttributeReferences : public vespalib::ObjectOperation, public vespalib::ObjectPredicate { public: - CheckAttributeReferences(bool log=false) : _log(log), _numrefs(0) { } + CheckAttributeReferences() : CheckAttributeReferences(false) {} + explicit CheckAttributeReferences(bool log) : _log(log), _numrefs(0) { } bool _log; uint32_t _numrefs; private: @@ -177,7 +178,7 @@ TEST_F("testGroupingContextInitialization", DoomFixture()) { baseRequest.serialize(nos); AllocatedBitVector bv(1); - GroupingContext context(bv, f1.clock.clock(), f1.timeOfDoom, os.data(), os.size(), true); + GroupingContext context(bv, f1.clock.clock(), f1.timeOfDoom, os.data(), os.size()); ASSERT_TRUE(!context.empty()); GroupingContext::GroupingList list = context.getGroupingList(); ASSERT_TRUE(list.size() == 1); @@ -303,8 +304,8 @@ TEST_F("testGroupingSession", DoomFixture()) { manager.groupInRelevanceOrder(&hit, 1); CheckAttributeReferences attrCheck_after; GroupingList &gl3(initContext.getGroupingList()); - for (unsigned int i = 0; i < gl3.size(); i++) { - gl3[i]->select(attrCheck_after, attrCheck_after); + for (auto & grouping : gl3) { + grouping->select(attrCheck_after, attrCheck_after); } EXPECT_EQUAL(attrCheck_after._numrefs, 0u); { @@ -423,9 +424,9 @@ void doGrouping(GroupingContext &ctx, { GroupingManager man(ctx); std::vector<RankedHit> hits; - hits.push_back(RankedHit(doc1, rank1)); - hits.push_back(RankedHit(doc2, rank2)); - hits.push_back(RankedHit(doc3, rank3)); + hits.emplace_back(doc1, rank1); + hits.emplace_back(doc2, rank2); + hits.emplace_back(doc3, rank3); man.groupInRelevanceOrder(&hits[0], 3); } diff --git a/searchcore/src/tests/proton/matching/unpacking_iterators_optimizer/unpacking_iterators_optimizer_test.cpp b/searchcore/src/tests/proton/matching/unpacking_iterators_optimizer/unpacking_iterators_optimizer_test.cpp index eb222518710..2e1a28402a1 100644 --- a/searchcore/src/tests/proton/matching/unpacking_iterators_optimizer/unpacking_iterators_optimizer_test.cpp +++ b/searchcore/src/tests/proton/matching/unpacking_iterators_optimizer/unpacking_iterators_optimizer_test.cpp @@ -254,12 +254,34 @@ std::string split_query_tree_dump = " Term a cheap\n" " Term b cheap\n" " Term c cheap\n"; +std::string split_query_tree_dump_always_expensive = + "And 7\n" + " Or 3\n" + " Term t2\n" + " Phrase 3 expensive\n" + " Term a\n" + " Term b\n" + " Term c\n" + " Term x1\n" + " Term x2\n" + " Phrase 3 expensive\n" + " Term a\n" + " Term b\n" + " Term c\n" + " Term t1\n" + " Term a cheap\n" + " Term b cheap\n" + " Term c cheap\n"; #endif //----------------------------------------------------------------------------- Node::UP optimize(Node::UP root, bool white_list) { - return UnpackingIteratorsOptimizer::optimize(std::move(root), white_list); + return UnpackingIteratorsOptimizer::optimize(std::move(root), white_list, false); +} + +Node::UP optimize(Node::UP root, bool white_list, bool always_mark_phrase_expensive) { + return UnpackingIteratorsOptimizer::optimize(std::move(root), white_list, always_mark_phrase_expensive); } TEST(UnpackingIteratorsOptimizerTest, require_that_root_phrase_node_can_be_left_alone) { @@ -301,4 +323,14 @@ TEST(UnpackingIteratorsOptimizerTest, require_that_query_tree_can_be_split) { EXPECT_EQ(actual2, expect); } +TEST(UnpackingIteratorsOptimizerTest, require_that_query_tree_can_be_split_always) { + std::string actual1 = dump_query(*optimize(make_query_tree(), false, false)); + std::string actual2 = dump_query(*optimize(make_query_tree(), true, false)); + std::string actual3 = dump_query(*optimize(make_query_tree(), true, true)); + std::string expect = split_query_tree_dump; + EXPECT_EQ(actual1, expect); + EXPECT_EQ(actual2, expect); + EXPECT_EQ(actual3, split_query_tree_dump_always_expensive); +} + GTEST_MAIN_RUN_ALL_TESTS() diff --git a/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp b/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp index 63127d01450..882a4d1509d 100644 --- a/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp +++ b/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp @@ -2,7 +2,6 @@ #include "groupingcontext.h" #include <vespa/searchlib/aggregation/predicates.h> -#include <vespa/searchlib/aggregation/modifiers.h> #include <vespa/searchlib/aggregation/hitsaggregationresult.h> #include <vespa/searchlib/common/bitvector.h> @@ -53,13 +52,12 @@ GroupingContext::setDistributionKey(uint32_t distributionKey) } GroupingContext::GroupingContext(const BitVector & validLids, const vespalib::Clock & clock, vespalib::steady_time timeOfDoom, - const char *groupSpec, uint32_t groupSpecLen, bool enableNested) + const char *groupSpec, uint32_t groupSpecLen) : _validLids(validLids), _clock(clock), _timeOfDoom(timeOfDoom), _os(), - _groupingList(), - _enableNestedMultivalueGrouping(enableNested) + _groupingList() { deserialize(groupSpec, groupSpecLen); } @@ -69,8 +67,7 @@ GroupingContext::GroupingContext(const BitVector & validLids, const vespalib::Cl _clock(clock), _timeOfDoom(timeOfDoom), _os(), - _groupingList(), - _enableNestedMultivalueGrouping(true) + _groupingList() { } GroupingContext::GroupingContext(const GroupingContext & rhs) @@ -78,8 +75,7 @@ GroupingContext::GroupingContext(const GroupingContext & rhs) _clock(rhs._clock), _timeOfDoom(rhs._timeOfDoom), _os(), - _groupingList(), - _enableNestedMultivalueGrouping(rhs._enableNestedMultivalueGrouping) + _groupingList() { } void @@ -100,7 +96,7 @@ GroupingContext::serialize() } bool -GroupingContext::needRanking() const +GroupingContext::needRanking() const noexcept { if (_groupingList.empty()) { return false; diff --git a/searchcore/src/vespa/searchcore/grouping/groupingcontext.h b/searchcore/src/vespa/searchcore/grouping/groupingcontext.h index c29b5122f74..e1b296df99b 100644 --- a/searchcore/src/vespa/searchcore/grouping/groupingcontext.h +++ b/searchcore/src/vespa/searchcore/grouping/groupingcontext.h @@ -34,7 +34,7 @@ public: * @param groupSpecLen The length of the grouping specification, in bytes. **/ GroupingContext(const BitVector & validLids, const vespalib::Clock & clock, vespalib::steady_time timeOfDoom, - const char *groupSpec, uint32_t groupSpecLen, bool enableNestedMultivalueGrouping); + const char *groupSpec, uint32_t groupSpecLen); /** * Create a new grouping context from a byte buffer. @@ -105,9 +105,7 @@ public: * Figure out if ranking is necessary for any of the grouping requests here. * @return true if ranking is required. */ - bool needRanking() const; - bool enableNestedMultivalueGrouping() const noexcept { return _enableNestedMultivalueGrouping; } - const search::BitVector & getValidLids() const { return _validLids; } + bool needRanking() const noexcept; void groupUnordered(const RankedHit *searchResults, uint32_t binSize, const search::BitVector * overflow); void groupInRelevanceOrder(const RankedHit *searchResults, uint32_t binSize); @@ -123,7 +121,6 @@ private: vespalib::steady_time _timeOfDoom; vespalib::nbostream _os; GroupingList _groupingList; - bool _enableNestedMultivalueGrouping; }; } diff --git a/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp b/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp index f5bcc935cb1..799d2663589 100644 --- a/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp +++ b/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp @@ -1,7 +1,6 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "groupingmanager.h" -#include "groupingsession.h" #include "groupingcontext.h" #include <vespa/searchlib/aggregation/fs4hit.h> #include <vespa/searchlib/expression/attributenode.h> @@ -52,11 +51,11 @@ GroupingManager::init(const IAttributeContext &attrCtx) ExpressionNode & en = *level.getExpression().getRoot(); if (en.inherits(AttributeNode::classId)) { - AttributeNode & an = static_cast<AttributeNode &>(en); + auto & an = static_cast<AttributeNode &>(en); an.enableEnumOptimization(true); } } - ConfigureStaticParams stuff(&attrCtx, nullptr, _groupingContext.enableNestedMultivalueGrouping()); + ConfigureStaticParams stuff(&attrCtx, nullptr); grouping.configureStaticStuff(stuff); list.push_back(groupingList[i]); } catch (const std::exception & e) { @@ -96,10 +95,9 @@ void GroupingManager::prune() { GroupingContext::GroupingList &groupingList(_groupingContext.getGroupingList()); - for (size_t i = 0; i < groupingList.size(); ++i) { - Grouping &g = *groupingList[i]; - g.postMerge(); - g.sortById(); + for (const auto & g : groupingList) { + g->postMerge(); + g->sortById(); } } @@ -107,10 +105,9 @@ void GroupingManager::convertToGlobalId(const search::IDocumentMetaStore &metaStore) { GroupingContext::GroupingList & groupingList = _groupingContext.getGroupingList(); - for (size_t i = 0; i < groupingList.size(); ++i) { - Grouping & g = *groupingList[i]; - g.convertToGlobalId(metaStore); - LOG(debug, "convertToGlobalId: %s", g.asString().c_str()); + for (const auto & g : groupingList) { + g->convertToGlobalId(metaStore); + LOG(debug, "convertToGlobalId: %s", g->asString().c_str()); } } diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp index 7e7532a3182..7beecaca613 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp @@ -193,7 +193,8 @@ MatchToolsFactory(QueryLimiter & queryLimiter, trace.addEvent(4, "Start query setup"); _query.setWhiteListBlueprint(metaStore.createWhiteListBlueprint()); trace.addEvent(5, "Deserialize and build query tree"); - _valid = _query.buildTree(queryStack, location, viewResolver, indexEnv); + _valid = _query.buildTree(queryStack, location, viewResolver, indexEnv, + AlwaysMarkPhraseExpensive::check(_queryEnv.getProperties(), rankSetup.always_mark_phrase_expensive())); if (_valid) { _query.extractTerms(_queryEnv.terms()); _query.extractLocations(_queryEnv.locations()); diff --git a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp index 3c8c90a229d..3e8909aa593 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp @@ -249,7 +249,7 @@ Matcher::match(const SearchRequest &request, vespalib::ThreadBundle &threadBundl { // we want to measure full set-up and tear-down time as part of // collateral time GroupingContext groupingContext(metaStore.getValidLids(), _clock, request.getTimeOfDoom(), - request.groupSpec.data(), request.groupSpec.size(), _rankSetup->enableNestedMultivalueGrouping()); + request.groupSpec.data(), request.groupSpec.size()); SessionId sessionId(request.sessionId.data(), request.sessionId.size()); bool shouldCacheSearchSession = false; bool shouldCacheGroupingSession = false; diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.cpp b/searchcore/src/vespa/searchcore/proton/matching/query.cpp index d4f4ae8015d..071e914b405 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/query.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/query.cpp @@ -165,7 +165,8 @@ Query::~Query() = default; bool Query::buildTree(vespalib::stringref stack, const string &location, - const ViewResolver &resolver, const IIndexEnvironment &indexEnv) + const ViewResolver &resolver, const IIndexEnvironment &indexEnv, + bool always_mark_phrase_expensive) { SimpleQueryStackDumpIterator stack_dump_iterator(stack); _query_tree = QueryTreeCreator<ProtonNodeTypes>::create(stack_dump_iterator); @@ -173,7 +174,7 @@ Query::buildTree(vespalib::stringref stack, const string &location, SameElementModifier prefixSameElementSubIndexes; _query_tree->accept(prefixSameElementSubIndexes); exchange_location_nodes(location, _query_tree, _locations); - _query_tree = UnpackingIteratorsOptimizer::optimize(std::move(_query_tree), bool(_whiteListBlueprint)); + _query_tree = UnpackingIteratorsOptimizer::optimize(std::move(_query_tree), bool(_whiteListBlueprint), always_mark_phrase_expensive); ResolveViewVisitor resolve_visitor(resolver, indexEnv); _query_tree->accept(resolve_visitor); return true; diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.h b/searchcore/src/vespa/searchcore/proton/matching/query.h index b67672ec3ef..6ea326834a5 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/query.h +++ b/searchcore/src/vespa/searchcore/proton/matching/query.h @@ -55,7 +55,15 @@ public: bool buildTree(vespalib::stringref stack, const vespalib::string &location, const ViewResolver &resolver, - const search::fef::IIndexEnvironment &idxEnv); + const search::fef::IIndexEnvironment &idxEnv) + { + return buildTree(stack, location, resolver, idxEnv, false); + } + bool buildTree(vespalib::stringref stack, + const vespalib::string &location, + const ViewResolver &resolver, + const search::fef::IIndexEnvironment &idxEnv, + bool always_mark_phrase_expensive); /** * Extract query terms from the query tree; to be used to build diff --git a/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.cpp b/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.cpp index e8dc8ab85ba..c9cfbbfd40e 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.cpp @@ -73,6 +73,8 @@ struct TermExpander : QueryVisitor { struct NodeTraverser : TemplateTermVisitor<NodeTraverser, ProtonNodeTypes> { + bool _always_mark_phrase_expensive; + NodeTraverser(bool always_mark_phrase_expensive) : _always_mark_phrase_expensive(always_mark_phrase_expensive) {} template <class TermNode> void visitTerm(TermNode &) {} void visit(ProtonNodeTypes::And &n) override { for (Node *child: n.getChildren()) { @@ -84,14 +86,19 @@ struct NodeTraverser : TemplateTermVisitor<NodeTraverser, ProtonNodeTypes> } expander.flush(n); } + void visit(Phrase &n) override { + if (_always_mark_phrase_expensive) { + n.set_expensive(true); + } + } }; } // namespace proton::matching::<unnamed> search::query::Node::UP -UnpackingIteratorsOptimizer::optimize(search::query::Node::UP root, bool has_white_list) +UnpackingIteratorsOptimizer::optimize(search::query::Node::UP root, bool has_white_list, bool always_mark_phrase_expensive) { - NodeTraverser traverser; + NodeTraverser traverser(always_mark_phrase_expensive); root->accept(traverser); if (has_white_list) { TermExpander expander; diff --git a/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.h b/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.h index f698b79dd0c..fc08ae3cfdd 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.h +++ b/searchcore/src/vespa/searchcore/proton/matching/unpacking_iterators_optimizer.h @@ -12,7 +12,7 @@ namespace proton::matching { * expensive. **/ struct UnpackingIteratorsOptimizer { - static search::query::Node::UP optimize(search::query::Node::UP root, bool has_white_list); + static search::query::Node::UP optimize(search::query::Node::UP root, bool has_white_list, bool always_mark_phrase_expensive); }; } diff --git a/searchlib/abi-spec.json b/searchlib/abi-spec.json index 0b1cb7a103c..2d67abb0e04 100644 --- a/searchlib/abi-spec.json +++ b/searchlib/abi-spec.json @@ -1728,4 +1728,4 @@ ], "fields" : [ ] } -} +}
\ No newline at end of file diff --git a/searchlib/src/tests/fef/properties/properties_test.cpp b/searchlib/src/tests/fef/properties/properties_test.cpp index 5e18c41b40a..c8073739b3e 100644 --- a/searchlib/src/tests/fef/properties/properties_test.cpp +++ b/searchlib/src/tests/fef/properties/properties_test.cpp @@ -10,9 +10,8 @@ using namespace search::fef::indexproperties; struct CopyVisitor : public IPropertiesVisitor { Properties &dst; - CopyVisitor(Properties &p) : dst(p) {} - virtual void visitProperty(const Property::Value &key, - const Property &values) override + explicit CopyVisitor(Properties &p) noexcept : dst(p) {} + void visitProperty(const Property::Value &key, const Property &values) override { for (uint32_t i = 0; i < values.size(); ++i) { dst.add(key, values.getAt(i)); @@ -590,5 +589,41 @@ TEST("test query feature type properties") EXPECT_EQUAL("", type::QueryFeature::lookup(p, "bar")); } +TEST("test integer lookup") +{ + EXPECT_EQUAL(matching::NumThreadsPerSearch::NAME, vespalib::string("vespa.matching.numthreadspersearch")); + EXPECT_EQUAL(matching::NumThreadsPerSearch::DEFAULT_VALUE, std::numeric_limits<uint32_t>::max()); + { + Properties p; + p.add("vespa.matching.numthreadspersearch", "50"); + EXPECT_EQUAL(matching::NumThreadsPerSearch::lookup(p), 50u); + } + { + Properties p; + p.add("vespa.matching.numthreadspersearch", "50 "); + EXPECT_EQUAL(matching::NumThreadsPerSearch::lookup(p), 50u); + } + { + Properties p; + p.add("vespa.matching.numthreadspersearch", " 50"); + EXPECT_EQUAL(matching::NumThreadsPerSearch::lookup(p), 50u); + } + { + Properties p; + p.add("vespa.matching.numthreadspersearch", " "); + EXPECT_EQUAL(matching::NumThreadsPerSearch::lookup(p), matching::NumThreadsPerSearch::DEFAULT_VALUE); + } + { + Properties p; + p.add("vespa.matching.numthreadspersearch", "50x"); + EXPECT_EQUAL(matching::NumThreadsPerSearch::lookup(p), 50u); + } + { + Properties p; + p.add("vespa.matching.numthreadspersearch", "x"); + EXPECT_EQUAL(matching::NumThreadsPerSearch::lookup(p), matching::NumThreadsPerSearch::DEFAULT_VALUE); + } +} + TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/searchlib/src/tests/queryeval/blueprint/intermediate_blueprints_test.cpp b/searchlib/src/tests/queryeval/blueprint/intermediate_blueprints_test.cpp index b150f8db7c5..51e22dbcf2c 100644 --- a/searchlib/src/tests/queryeval/blueprint/intermediate_blueprints_test.cpp +++ b/searchlib/src/tests/queryeval/blueprint/intermediate_blueprints_test.cpp @@ -119,7 +119,9 @@ TEST("test And propagates updated histestimate") { bp.addChild(ap(MyLeafSpec(20).create<RememberExecuteInfo>()->setSourceId(2))); bp.addChild(ap(MyLeafSpec(200).create<RememberExecuteInfo>()->setSourceId(2))); bp.addChild(ap(MyLeafSpec(2000).create<RememberExecuteInfo>()->setSourceId(2))); - bp.optimize_self(); + bp.optimize_self(Blueprint::OptimizePass::FIRST); + bp.optimize_self(Blueprint::OptimizePass::SECOND); + bp.optimize_self(Blueprint::OptimizePass::LAST); bp.setDocIdLimit(5000); bp.fetchPostings(ExecuteInfo::TRUE); EXPECT_EQUAL(3u, bp.childCnt()); @@ -139,7 +141,9 @@ TEST("test Or propagates updated histestimate") { bp.addChild(ap(MyLeafSpec(2000).create<RememberExecuteInfo>()->setSourceId(2))); bp.addChild(ap(MyLeafSpec(800).create<RememberExecuteInfo>()->setSourceId(2))); bp.addChild(ap(MyLeafSpec(20).create<RememberExecuteInfo>()->setSourceId(2))); - bp.optimize_self(); + bp.optimize_self(Blueprint::OptimizePass::FIRST); + bp.optimize_self(Blueprint::OptimizePass::SECOND); + bp.optimize_self(Blueprint::OptimizePass::LAST); bp.setDocIdLimit(5000); bp.fetchPostings(ExecuteInfo::TRUE); EXPECT_EQUAL(4u, bp.childCnt()); @@ -1415,22 +1419,57 @@ TEST("require that highest cost tier sorts last for AND") { EXPECT_EQUAL(expect_up->asString(), top_up->asString()); } -TEST("require that intermediate cost tier is minimum cost tier of children") { - Blueprint::UP bp1( - ap((new AndBlueprint())-> - addChild(ap(MyLeafSpec(10).cost_tier(1).create())). - addChild(ap(MyLeafSpec(20).cost_tier(2).create())). - addChild(ap(MyLeafSpec(30).cost_tier(3).create())))); - Blueprint::UP bp2( - ap((new AndBlueprint())-> - addChild(ap(MyLeafSpec(10).cost_tier(3).create())). - addChild(ap(MyLeafSpec(20).cost_tier(2).create())). - addChild(ap(MyLeafSpec(30).cost_tier(2).create())))); - EXPECT_EQUAL(bp1->getState().cost_tier(), 1u); - EXPECT_EQUAL(bp2->getState().cost_tier(), 2u); +template<typename BP> +void +verifyCostTierInheritance(uint8_t expected, uint8_t expected_reverse) { + auto bp1 = std::make_unique<BP>(); + bp1->addChild(ap(MyLeafSpec(10).cost_tier(1).create())). + addChild(ap(MyLeafSpec(20).cost_tier(2).create())). + addChild(ap(MyLeafSpec(30).cost_tier(3).create())); + auto bp2 = std::make_unique<BP>(); + bp2->addChild(ap(MyLeafSpec(10).cost_tier(3).create())). + addChild(ap(MyLeafSpec(20).cost_tier(2).create())). + addChild(ap(MyLeafSpec(30).cost_tier(1).create())); + EXPECT_EQUAL(bp1->getState().cost_tier(), expected); + EXPECT_EQUAL(bp2->getState().cost_tier(), expected_reverse); +} + +TEST("require that AND cost tier is minimum cost tier of children") { + verifyCostTierInheritance<AndBlueprint>(1, 1); +} + +TEST("require that OR cost tier is maximum cost tier of children") { + verifyCostTierInheritance<OrBlueprint>(3, 3); +} + +TEST("require that Rank cost tier is first childs cost tier") { + verifyCostTierInheritance<RankBlueprint>(1, 3); +} + +TEST("require that AndNot cost tier is first childs cost tier") { + verifyCostTierInheritance<AndNotBlueprint>(1, 3); +} + +struct MySourceBlender { + InvalidSelector selector; + SourceBlenderBlueprint sb; + MySourceBlender() : selector(), sb(selector) {} + IntermediateBlueprint & + addChild(Blueprint::UP child) { + return sb.addChild(std::move(child)); + } + const Blueprint::State &getState() const { + return sb.getState(); + } + +}; + +TEST("require that SourceBlender cost tier is maximum cost tier of children") { + verifyCostTierInheritance<MySourceBlender>(3, 3); } -void verify_or_est(const std::vector<Blueprint::HitEstimate> &child_estimates, Blueprint::HitEstimate expect) { +void +verify_or_est(const std::vector<Blueprint::HitEstimate> &child_estimates, Blueprint::HitEstimate expect) { OrBlueprint my_or; my_or.setDocIdLimit(32); auto my_est = my_or.combine(child_estimates); diff --git a/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp b/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp index 71033ed7d06..5933122d7a2 100644 --- a/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp +++ b/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp @@ -47,7 +47,7 @@ concept ChildCollector = requires(T a, std::unique_ptr<Blueprint> bp) { // inherit Blueprint to capture the default filter factory struct DefaultBlueprint : Blueprint { - void optimize(Blueprint* &) override { abort(); } + void optimize(Blueprint* &, OptimizePass) override { abort(); } const State &getState() const override { abort(); } void fetchPostings(const ExecuteInfo &) override { abort(); } void freeze() override { abort(); } diff --git a/searchlib/src/vespa/searchlib/expression/expressiontree.h b/searchlib/src/vespa/searchlib/expression/expressiontree.h index 52c075f3e29..127de66b6dc 100644 --- a/searchlib/src/vespa/searchlib/expression/expressiontree.h +++ b/searchlib/src/vespa/searchlib/expression/expressiontree.h @@ -50,11 +50,11 @@ public: }; ExpressionTree() noexcept; - ExpressionTree(const ExpressionNode & root); - ExpressionTree(ExpressionNode::UP root); + explicit ExpressionTree(const ExpressionNode & root); + explicit ExpressionTree(ExpressionNode::UP root); ExpressionTree(const ExpressionTree & rhs); ExpressionTree(ExpressionTree &&) noexcept = default; - ~ExpressionTree(); + ~ExpressionTree() override; ExpressionTree & operator = (ExpressionNode::UP rhs); ExpressionTree & operator = (const ExpressionTree & rhs); ExpressionTree & operator = (ExpressionTree &&) noexcept = default; diff --git a/searchlib/src/vespa/searchlib/fef/indexproperties.cpp b/searchlib/src/vespa/searchlib/fef/indexproperties.cpp index 9b111c4bd5d..9c986d0bc63 100644 --- a/searchlib/src/vespa/searchlib/fef/indexproperties.cpp +++ b/searchlib/src/vespa/searchlib/fef/indexproperties.cpp @@ -4,6 +4,7 @@ #include "properties.h" #include <vespa/vespalib/locale/c.h> #include <limits> +#include <charconv> namespace search::fef::indexproperties { @@ -49,10 +50,15 @@ uint32_t lookupUint32(const Properties &props, const vespalib::string &name, uint32_t defaultValue) { Property p = props.lookup(name); + uint32_t value(defaultValue); if (p.found()) { - return atoi(p.get().c_str()); + const auto & valS = p.get(); + const char * start = valS.c_str(); + const char * end = start + valS.size(); + while ((start != end) && isspace(start[0])) { start++; } + std::from_chars(start, end, value); } - return defaultValue; + return value; } bool @@ -173,11 +179,6 @@ namespace onsummary { namespace temporary { -const vespalib::string EnableNestedMultivalueGrouping::NAME("vespa.temporary.enable_nested_multivalue_grouping"); -bool EnableNestedMultivalueGrouping::check(const Properties &props) { - return lookupBool(props, NAME, false); -} - } namespace mutate { @@ -454,6 +455,12 @@ FuzzyAlgorithm::lookup(const Properties& props, vespalib::FuzzyMatchingAlgorithm return vespalib::fuzzy_matching_algorithm_from_string(value, default_value); } +const vespalib::string AlwaysMarkPhraseExpensive::NAME("vespa.matching.always_mark_phrase_expensive"); +const bool AlwaysMarkPhraseExpensive::DEFAULT_VALUE(false); +bool AlwaysMarkPhraseExpensive::check(const Properties &props, bool fallback) { + return lookupBool(props, NAME, fallback); +} + } // namespace matching namespace softtimeout { diff --git a/searchlib/src/vespa/searchlib/fef/indexproperties.h b/searchlib/src/vespa/searchlib/fef/indexproperties.h index c528c4366d6..1921f52276f 100644 --- a/searchlib/src/vespa/searchlib/fef/indexproperties.h +++ b/searchlib/src/vespa/searchlib/fef/indexproperties.h @@ -176,11 +176,8 @@ namespace mutate { }; } +// Add temporary flags used for safe rollout of new features here namespace temporary { - struct EnableNestedMultivalueGrouping { - static const vespalib::string NAME; - static bool check(const Properties &props); - }; } namespace mutate::on_match { @@ -339,6 +336,17 @@ namespace matching { static vespalib::FuzzyMatchingAlgorithm lookup(const Properties& props); static vespalib::FuzzyMatchingAlgorithm lookup(const Properties& props, vespalib::FuzzyMatchingAlgorithm default_value); }; + + /** + * When enabled, the unpacking part of the phrase iterator will be tagged as expensive + * under all intermediate iterators, not only AND. + **/ + struct AlwaysMarkPhraseExpensive { + static const vespalib::string NAME; + static const bool DEFAULT_VALUE; + static bool check(const Properties &props) { return check(props, DEFAULT_VALUE); } + static bool check(const Properties &props, bool fallback); + }; } namespace softtimeout { diff --git a/searchlib/src/vespa/searchlib/fef/properties.cpp b/searchlib/src/vespa/searchlib/fef/properties.cpp index bd4795fcc5a..6134807fa60 100644 --- a/searchlib/src/vespa/searchlib/fef/properties.cpp +++ b/searchlib/src/vespa/searchlib/fef/properties.cpp @@ -25,7 +25,7 @@ uint32_t Properties::rawHash(const void *buf, uint32_t len) noexcept { uint32_t res = 0; - unsigned const char *pt = (unsigned const char *) buf; + auto *pt = (unsigned const char *) buf; unsigned const char *end = pt + len; while (pt < end) { res = (res << 7) + (res >> 25) + *pt++; @@ -52,7 +52,7 @@ Properties::add(vespalib::stringref key, vespalib::stringref value) { if (!key.empty()) { Value & v = _data[key]; - v.push_back(value); + v.emplace_back(value); ++_numValues; } return *this; @@ -162,20 +162,20 @@ Property Properties::lookup(vespalib::stringref key) const noexcept { if (key.empty()) { - return Property(); + return {}; } auto node = _data.find(key); if (node == _data.end()) { - return Property(); + return {}; } - return Property(node->second); + return {node->second}; } Property Properties::lookup(vespalib::stringref namespace1, vespalib::stringref key) const noexcept { if (namespace1.empty() || key.empty()) { - return Property(); + return {}; } vespalib::string fullKey(namespace1); fullKey.append('.').append(key); @@ -187,7 +187,7 @@ Property Properties::lookup(vespalib::stringref namespace1, vespalib::stringref key) const noexcept { if (namespace1.empty() || namespace2.empty() || key.empty()) { - return Property(); + return {}; } vespalib::string fullKey(namespace1); fullKey.append('.').append(namespace2).append('.').append(key); @@ -200,7 +200,7 @@ Property Properties::lookup(vespalib::stringref namespace1, vespalib::stringref key) const noexcept { if (namespace1.empty() || namespace2.empty() || namespace3.empty() || key.empty()) { - return Property(); + return {}; } vespalib::string fullKey(namespace1); fullKey.append('.').append(namespace2).append('.').append(namespace3).append('.').append(key); diff --git a/searchlib/src/vespa/searchlib/fef/ranksetup.cpp b/searchlib/src/vespa/searchlib/fef/ranksetup.cpp index 33e7dbda04b..d6b0b900516 100644 --- a/searchlib/src/vespa/searchlib/fef/ranksetup.cpp +++ b/searchlib/src/vespa/searchlib/fef/ranksetup.cpp @@ -60,6 +60,7 @@ RankSetup::RankSetup(const BlueprintFactory &factory, const IIndexEnvironment &i _compiled(false), _compileError(false), _degradationAscendingOrder(false), + _always_mark_phrase_expensive(false), _diversityAttribute(), _diversityMinGroups(1), _diversityCutoffFactor(10.0), @@ -74,8 +75,7 @@ RankSetup::RankSetup(const BlueprintFactory &factory, const IIndexEnvironment &i _mutateOnFirstPhase(), _mutateOnSecondPhase(), _mutateOnSummary(), - _mutateAllowQueryOverride(false), - _enableNestedMultivalueGrouping(false) + _mutateAllowQueryOverride(false) { } RankSetup::~RankSetup() = default; @@ -134,7 +134,7 @@ RankSetup::configure() _mutateOnSummary._attribute = mutate::on_summary::Attribute::lookup(_indexEnv.getProperties()); _mutateOnSummary._operation = mutate::on_summary::Operation::lookup(_indexEnv.getProperties()); _mutateAllowQueryOverride = mutate::AllowQueryOverride::check(_indexEnv.getProperties()); - _enableNestedMultivalueGrouping = temporary::EnableNestedMultivalueGrouping::check(_indexEnv.getProperties()); + _always_mark_phrase_expensive = matching::AlwaysMarkPhraseExpensive::check(_indexEnv.getProperties()); } void diff --git a/searchlib/src/vespa/searchlib/fef/ranksetup.h b/searchlib/src/vespa/searchlib/fef/ranksetup.h index 6f4651939ad..d744b38cc6e 100644 --- a/searchlib/src/vespa/searchlib/fef/ranksetup.h +++ b/searchlib/src/vespa/searchlib/fef/ranksetup.h @@ -32,7 +32,7 @@ public: : _attribute(attribute), _operation(operation) {} - bool enabled() const { return !_attribute.empty() && !_operation.empty(); } + bool enabled() const noexcept { return !_attribute.empty() && !_operation.empty(); } vespalib::string _attribute; vespalib::string _operation; }; @@ -69,6 +69,7 @@ private: bool _compiled; bool _compileError; bool _degradationAscendingOrder; + bool _always_mark_phrase_expensive; vespalib::string _diversityAttribute; uint32_t _diversityMinGroups; double _diversityCutoffFactor; @@ -84,7 +85,6 @@ private: MutateOperation _mutateOnSecondPhase; MutateOperation _mutateOnSummary; bool _mutateAllowQueryOverride; - bool _enableNestedMultivalueGrouping; void compileAndCheckForErrors(BlueprintResolver &bp); public: @@ -221,6 +221,7 @@ public: bool isDegradationOrderAscending() const { return _degradationAscendingOrder; } + bool always_mark_phrase_expensive() const noexcept { return _always_mark_phrase_expensive; } /** get number of hits to collect during graceful degradation in match phase */ uint32_t getDegradationMaxHits() const { return _degradationMaxHits; @@ -458,7 +459,6 @@ public: const MutateOperation & getMutateOnSummary() const { return _mutateOnSummary; } bool allowMutateQueryOverride() const { return _mutateAllowQueryOverride; } - bool enableNestedMultivalueGrouping() const { return _enableNestedMultivalueGrouping; } }; } diff --git a/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.cpp b/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.cpp index fd812708ebc..ac54ea205be 100644 --- a/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.cpp +++ b/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.cpp @@ -10,28 +10,6 @@ using search::query::PredicateQueryTerm; namespace search { -namespace { - -uint64_t -readUint64(const char *&p) -{ - uint64_t value; - memcpy(&value, p, sizeof(value)); - p += sizeof(value); - return vespalib::nbo::n2h(value); -} - -double -read_double(const char *&p) -{ - double value; - memcpy(&value, p, sizeof(value)); - p += sizeof(value); - return vespalib::nbo::n2h(value); -} - -} - SimpleQueryStackDumpIterator::SimpleQueryStackDumpIterator(vespalib::stringref buf) : _buf(buf.begin()), _bufEnd(buf.end()), @@ -90,6 +68,19 @@ SimpleQueryStackDumpIterator::readCompressedInt(const char *&p) return tmp; } +template <typename T> +T +SimpleQueryStackDumpIterator::read_value(const char *&p) +{ + T value; + if (p + sizeof(value) > _bufEnd) { + throw false; + } + memcpy(&value, p, sizeof(value)); + p += sizeof(value); + return vespalib::nbo::n2h(value); +} + bool SimpleQueryStackDumpIterator::next() { try { @@ -167,13 +158,8 @@ bool SimpleQueryStackDumpIterator::readNext() { _currArity = 0; break; case ParseItem::ITEM_PURE_WEIGHTED_LONG: - { - if (p + sizeof(int64_t) > _bufEnd) { - return false; - } - _curr_integer_term = readUint64(p); - _currArity = 0; - } + _curr_integer_term = read_value<int64_t>(p); + _currArity = 0; break; case ParseItem::ITEM_WORD_ALTERNATIVES: _curr_index_name = read_stringref(p); @@ -193,20 +179,20 @@ bool SimpleQueryStackDumpIterator::readNext() { _currArity = 0; break; case ParseItem::ITEM_PREDICATE_QUERY: - if ( ! readPredicate(p)) return false; + readPredicate(p); break; case ParseItem::ITEM_WEIGHTED_SET: case ParseItem::ITEM_DOT_PRODUCT: case ParseItem::ITEM_WAND: case ParseItem::ITEM_PHRASE: - if (!readComplexTerm(p)) return false; + readComplexTerm(p); break; case ParseItem::ITEM_NEAREST_NEIGHBOR: - if ( ! readNN(p)) return false; + readNN(p); break; case ParseItem::ITEM_FUZZY: - if (!readFuzzy(p)) return false; + readFuzzy(p); break; case ParseItem::ITEM_TRUE: case ParseItem::ITEM_FALSE: @@ -223,7 +209,7 @@ bool SimpleQueryStackDumpIterator::readNext() { return (p <= _bufEnd); } -bool +void SimpleQueryStackDumpIterator::readPredicate(const char *&p) { _curr_index_name = read_stringref(p); _predicate_query_term = std::make_unique<PredicateQueryTerm>(); @@ -232,22 +218,19 @@ SimpleQueryStackDumpIterator::readPredicate(const char *&p) { for (size_t i = 0; i < count; ++i) { vespalib::stringref key = read_stringref(p); vespalib::stringref value = read_stringref(p); - if (p + sizeof(uint64_t) > _bufEnd) return false; - uint64_t sub_queries = readUint64(p); + uint64_t sub_queries = read_value<uint64_t>(p); _predicate_query_term->addFeature(key, value, sub_queries); } count = readCompressedPositiveInt(p); for (size_t i = 0; i < count; ++i) { vespalib::stringref key = read_stringref(p); - if (p + 2*sizeof(uint64_t) > _bufEnd) return false; - uint64_t value = readUint64(p); - uint64_t sub_queries = readUint64(p); + uint64_t value = read_value<uint64_t>(p); + uint64_t sub_queries = read_value<uint64_t>(p); _predicate_query_term->addRangeFeature(key, value, sub_queries); } - return true; } -bool +void SimpleQueryStackDumpIterator::readNN(const char *& p) { _curr_index_name = read_stringref(p); _curr_term = read_stringref(p); // query_tensor_name @@ -257,34 +240,29 @@ SimpleQueryStackDumpIterator::readNN(const char *& p) { // XXX: remove later when QRS doesn't send this extra flag _extraIntArg2 &= ~0x40; // QRS always sends this now: - if ((p + sizeof(double))> _bufEnd) return false; - _extraDoubleArg4 = read_double(p); // distance threshold + _extraDoubleArg4 = read_value<double>(p); // distance threshold _currArity = 0; - return true; } -bool +void SimpleQueryStackDumpIterator::readComplexTerm(const char *& p) { _currArity = readCompressedPositiveInt(p); _curr_index_name = read_stringref(p); if (_currType == ParseItem::ITEM_WAND) { _extraIntArg1 = readCompressedPositiveInt(p); // targetNumHits - if ((p + 2*sizeof(double))> _bufEnd) return false; - _extraDoubleArg4 = read_double(p); // scoreThreshold - _extraDoubleArg5 = read_double(p); // thresholdBoostFactor + _extraDoubleArg4 = read_value<double>(p); // scoreThreshold + _extraDoubleArg5 = read_value<double>(p); // thresholdBoostFactor } _curr_term = vespalib::stringref(); - return true; } -bool +void SimpleQueryStackDumpIterator::readFuzzy(const char *&p) { _curr_index_name = read_stringref(p); _curr_term = read_stringref(p); // fuzzy term _extraIntArg1 = readCompressedPositiveInt(p); // maxEditDistance _extraIntArg2 = readCompressedPositiveInt(p); // prefixLength _currArity = 0; - return true; } std::unique_ptr<query::PredicateQueryTerm> diff --git a/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.h b/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.h index 7d441dcef4b..add26ac7938 100644 --- a/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.h +++ b/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.h @@ -52,10 +52,12 @@ private: VESPA_DLL_LOCAL vespalib::stringref read_stringref(const char *&p); VESPA_DLL_LOCAL uint64_t readCompressedPositiveInt(const char *&p); VESPA_DLL_LOCAL int64_t readCompressedInt(const char *&p); - VESPA_DLL_LOCAL bool readPredicate(const char *&p); - VESPA_DLL_LOCAL bool readNN(const char *&p); - VESPA_DLL_LOCAL bool readComplexTerm(const char *& p); - VESPA_DLL_LOCAL bool readFuzzy(const char *&p); + template <typename T> + VESPA_DLL_LOCAL T read_value(const char*& p); + VESPA_DLL_LOCAL void readPredicate(const char *&p); + VESPA_DLL_LOCAL void readNN(const char *&p); + VESPA_DLL_LOCAL void readComplexTerm(const char *& p); + VESPA_DLL_LOCAL void readFuzzy(const char *&p); VESPA_DLL_LOCAL bool readNext(); public: /** diff --git a/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp b/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp index aba80faaa8a..5eaa2dc40ab 100644 --- a/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp +++ b/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp @@ -129,12 +129,14 @@ Blueprint::~Blueprint() = default; Blueprint::UP Blueprint::optimize(Blueprint::UP bp) { Blueprint *root = bp.release(); - root->optimize(root); + root->optimize(root, OptimizePass::FIRST); + root->optimize(root, OptimizePass::SECOND); + root->optimize(root, OptimizePass::LAST); return Blueprint::UP(root); } void -Blueprint::optimize_self() +Blueprint::optimize_self(OptimizePass) { } @@ -539,19 +541,23 @@ IntermediateBlueprint::should_do_termwise_eval(const UnpackInfo &unpack, double } void -IntermediateBlueprint::optimize(Blueprint* &self) +IntermediateBlueprint::optimize(Blueprint* &self, OptimizePass pass) { assert(self == this); if (should_optimize_children()) { for (auto &child : _children) { auto *child_ptr = child.release(); - child_ptr->optimize(child_ptr); + child_ptr->optimize(child_ptr, pass); child.reset(child_ptr); } } - optimize_self(); - sort(_children); - maybe_eliminate_self(self, get_replacement()); + optimize_self(pass); + if (pass == OptimizePass::LAST) { + sort(_children); + } + if (pass == OptimizePass::FIRST) { + maybe_eliminate_self(self, get_replacement()); + } } void @@ -725,11 +731,13 @@ LeafBlueprint::getRange(vespalib::string &, vespalib::string &) const { } void -LeafBlueprint::optimize(Blueprint* &self) +LeafBlueprint::optimize(Blueprint* &self, OptimizePass pass) { assert(self == this); - optimize_self(); - maybe_eliminate_self(self, get_replacement()); + optimize_self(pass); + if (pass == OptimizePass::FIRST) { + maybe_eliminate_self(self, get_replacement()); + } } void diff --git a/searchlib/src/vespa/searchlib/queryeval/blueprint.h b/searchlib/src/vespa/searchlib/queryeval/blueprint.h index da6050f075d..cd0e8f2af40 100644 --- a/searchlib/src/vespa/searchlib/queryeval/blueprint.h +++ b/searchlib/src/vespa/searchlib/queryeval/blueprint.h @@ -45,6 +45,8 @@ public: using Children = std::vector<Blueprint::UP>; using SearchIteratorUP = std::unique_ptr<SearchIterator>; + enum class OptimizePass { FIRST, SECOND, LAST }; + struct HitEstimate { uint32_t estHits; bool empty; @@ -206,8 +208,8 @@ public: uint32_t get_docid_limit() const noexcept { return _docid_limit; } static Blueprint::UP optimize(Blueprint::UP bp); - virtual void optimize(Blueprint* &self) = 0; - virtual void optimize_self(); + virtual void optimize(Blueprint* &self, OptimizePass pass) = 0; + virtual void optimize_self(OptimizePass pass); virtual Blueprint::UP get_replacement(); virtual bool should_optimize_children() const { return true; } @@ -298,7 +300,7 @@ class IntermediateBlueprint : public blueprint::StateCache private: Children _children; HitEstimate calculateEstimate() const; - uint8_t calculate_cost_tier() const; + virtual uint8_t calculate_cost_tier() const; uint32_t calculate_tree_size() const; bool infer_allow_termwise_eval() const; bool infer_want_global_filter() const; @@ -326,7 +328,7 @@ public: void setDocIdLimit(uint32_t limit) noexcept final; - void optimize(Blueprint* &self) final; + void optimize(Blueprint* &self, OptimizePass pass) final; void set_global_filter(const GlobalFilter &global_filter, double estimated_hit_ratio) override; IndexList find(const IPredicate & check) const; @@ -361,7 +363,7 @@ class LeafBlueprint : public Blueprint private: State _state; protected: - void optimize(Blueprint* &self) final; + void optimize(Blueprint* &self, OptimizePass pass) final; void setEstimate(HitEstimate est) { _state.estimate(est); notifyChange(); diff --git a/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.cpp b/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.cpp index 790d61b3731..c0439df1c1b 100644 --- a/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.cpp +++ b/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.cpp @@ -103,25 +103,27 @@ AndNotBlueprint::exposeFields() const } void -AndNotBlueprint::optimize_self() +AndNotBlueprint::optimize_self(OptimizePass pass) { if (childCnt() == 0) { return; } - if (getChild(0).isAndNot()) { - auto *child = static_cast<AndNotBlueprint *>(&getChild(0)); - while (child->childCnt() > 1) { - addChild(child->removeChild(1)); + if (pass == OptimizePass::FIRST) { + if (getChild(0).isAndNot()) { + auto *child = static_cast<AndNotBlueprint *>(&getChild(0)); + while (child->childCnt() > 1) { + addChild(child->removeChild(1)); + } + insertChild(1, child->removeChild(0)); + removeChild(0); } - insertChild(1, child->removeChild(0)); - removeChild(0); - } - for (size_t i = 1; i < childCnt(); ++i) { - if (getChild(i).getState().estimate().empty) { - removeChild(i--); + for (size_t i = 1; i < childCnt(); ++i) { + if (getChild(i).getState().estimate().empty) { + removeChild(i--); + } } } - if ( !(getParent() && getParent()->isAndNot()) ) { + if (pass == OptimizePass::LAST) { optimize_source_blenders<OrBlueprint>(*this, 1); } } @@ -191,18 +193,20 @@ AndBlueprint::exposeFields() const } void -AndBlueprint::optimize_self() -{ - for (size_t i = 0; i < childCnt(); ++i) { - if (getChild(i).isAnd()) { - auto *child = static_cast<AndBlueprint *>(&getChild(i)); - while (child->childCnt() > 0) { - addChild(child->removeChild(0)); +AndBlueprint::optimize_self(OptimizePass pass) +{ + if (pass == OptimizePass::FIRST) { + for (size_t i = 0; i < childCnt(); ++i) { + if (getChild(i).isAnd()) { + auto *child = static_cast<AndBlueprint *>(&getChild(i)); + while (child->childCnt() > 0) { + addChild(child->removeChild(0)); + } + removeChild(i--); } - removeChild(i--); } } - if ( !(getParent() && getParent()->isAnd()) ) { + if (pass == OptimizePass::LAST) { optimize_source_blenders<AndBlueprint>(*this, 0); } } @@ -291,20 +295,22 @@ OrBlueprint::exposeFields() const } void -OrBlueprint::optimize_self() -{ - for (size_t i = 0; (childCnt() > 1) && (i < childCnt()); ++i) { - if (getChild(i).isOr()) { - auto *child = static_cast<OrBlueprint *>(&getChild(i)); - while (child->childCnt() > 0) { - addChild(child->removeChild(0)); +OrBlueprint::optimize_self(OptimizePass pass) +{ + if (pass == OptimizePass::FIRST) { + for (size_t i = 0; (childCnt() > 1) && (i < childCnt()); ++i) { + if (getChild(i).isOr()) { + auto *child = static_cast<OrBlueprint *>(&getChild(i)); + while (child->childCnt() > 0) { + addChild(child->removeChild(0)); + } + removeChild(i--); + } else if (getChild(i).getState().estimate().empty) { + removeChild(i--); } - removeChild(i--); - } else if (getChild(i).getState().estimate().empty) { - removeChild(i--); } } - if ( !(getParent() && getParent()->isOr()) ) { + if (pass == OptimizePass::LAST) { optimize_source_blenders<OrBlueprint>(*this, 0); } } @@ -355,6 +361,16 @@ OrBlueprint::createFilterSearch(bool strict, FilterConstraint constraint) const return create_or_filter(get_children(), strict, constraint); } +uint8_t +OrBlueprint::calculate_cost_tier() const +{ + uint8_t cost_tier = State::COST_TIER_NORMAL; + for (const Blueprint::UP &child : get_children()) { + cost_tier = std::max(cost_tier, child->getState().cost_tier()); + } + return cost_tier; +} + //----------------------------------------------------------------------------- WeakAndBlueprint::~WeakAndBlueprint() = default; @@ -542,14 +558,18 @@ RankBlueprint::exposeFields() const } void -RankBlueprint::optimize_self() +RankBlueprint::optimize_self(OptimizePass pass) { - for (size_t i = 1; i < childCnt(); ++i) { - if (getChild(i).getState().estimate().empty) { - removeChild(i--); + if (pass == OptimizePass::FIRST) { + for (size_t i = 1; i < childCnt(); ++i) { + if (getChild(i).getState().estimate().empty) { + removeChild(i--); + } } } - optimize_source_blenders<OrBlueprint>(*this, 1); + if (pass == OptimizePass::LAST) { + optimize_source_blenders<OrBlueprint>(*this, 1); + } } Blueprint::UP @@ -663,6 +683,16 @@ SourceBlenderBlueprint::isCompatibleWith(const SourceBlenderBlueprint &other) co return (&_selector == &other._selector); } +uint8_t +SourceBlenderBlueprint::calculate_cost_tier() const +{ + uint8_t cost_tier = State::COST_TIER_NORMAL; + for (const Blueprint::UP &child : get_children()) { + cost_tier = std::max(cost_tier, child->getState().cost_tier()); + } + return cost_tier; +} + //----------------------------------------------------------------------------- } diff --git a/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.h b/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.h index 409a9e0fe95..75dc47272af 100644 --- a/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.h +++ b/searchlib/src/vespa/searchlib/queryeval/intermediate_blueprints.h @@ -17,7 +17,7 @@ public: bool supports_termwise_children() const override { return true; } HitEstimate combine(const std::vector<HitEstimate> &data) const override; FieldSpecBaseList exposeFields() const override; - void optimize_self() override; + void optimize_self(OptimizePass pass) override; bool isAndNot() const override { return true; } Blueprint::UP get_replacement() override; void sort(Children &children) const override; @@ -28,6 +28,9 @@ public: SearchIterator::UP createFilterSearch(bool strict, FilterConstraint constraint) const override; private: + uint8_t calculate_cost_tier() const override { + return (childCnt() > 0) ? get_children()[0]->getState().cost_tier() : State::COST_TIER_NORMAL; + } bool isPositive(size_t index) const override { return index == 0; } }; @@ -40,7 +43,7 @@ public: bool supports_termwise_children() const override { return true; } HitEstimate combine(const std::vector<HitEstimate> &data) const override; FieldSpecBaseList exposeFields() const override; - void optimize_self() override; + void optimize_self(OptimizePass pass) override; bool isAnd() const override { return true; } Blueprint::UP get_replacement() override; void sort(Children &children) const override; @@ -64,7 +67,7 @@ public: bool supports_termwise_children() const override { return true; } HitEstimate combine(const std::vector<HitEstimate> &data) const override; FieldSpecBaseList exposeFields() const override; - void optimize_self() override; + void optimize_self(OptimizePass pass) override; bool isOr() const override { return true; } Blueprint::UP get_replacement() override; void sort(Children &children) const override; @@ -76,6 +79,7 @@ public: createFilterSearch(bool strict, FilterConstraint constraint) const override; private: double computeNextHitRate(const Blueprint & child, double hitRate) const override; + uint8_t calculate_cost_tier() const override; }; //----------------------------------------------------------------------------- @@ -158,7 +162,7 @@ class RankBlueprint final : public IntermediateBlueprint public: HitEstimate combine(const std::vector<HitEstimate> &data) const override; FieldSpecBaseList exposeFields() const override; - void optimize_self() override; + void optimize_self(OptimizePass pass) override; Blueprint::UP get_replacement() override; void sort(Children &children) const override; bool inheritStrict(size_t i) const override; @@ -168,6 +172,9 @@ public: bool strict, fef::MatchData &md) const override; SearchIterator::UP createFilterSearch(bool strict, FilterConstraint constraint) const override; + uint8_t calculate_cost_tier() const override { + return (childCnt() > 0) ? get_children()[0]->getState().cost_tier() : State::COST_TIER_NORMAL; + } }; //----------------------------------------------------------------------------- @@ -193,6 +200,7 @@ public: /** check if this blueprint has the same source selector as the other */ bool isCompatibleWith(const SourceBlenderBlueprint &other) const; bool isSourceBlender() const override { return true; } + uint8_t calculate_cost_tier() const override; }; } diff --git a/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.cpp b/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.cpp index 6b2faac847e..5c795679d48 100644 --- a/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.cpp +++ b/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.cpp @@ -45,12 +45,14 @@ SameElementBlueprint::addTerm(Blueprint::UP term) } void -SameElementBlueprint::optimize_self() +SameElementBlueprint::optimize_self(OptimizePass pass) { - std::sort(_terms.begin(), _terms.end(), - [](const auto &a, const auto &b) { - return (a->getState().estimate() < b->getState().estimate()); - }); + if (pass == OptimizePass::LAST) { + std::sort(_terms.begin(), _terms.end(), + [](const auto &a, const auto &b) { + return (a->getState().estimate() < b->getState().estimate()); + }); + } } void diff --git a/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.h b/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.h index 240d064c5cc..cc6331375cf 100644 --- a/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.h +++ b/searchlib/src/vespa/searchlib/queryeval/same_element_blueprint.h @@ -34,7 +34,7 @@ public: // used by create visitor void addTerm(Blueprint::UP term); - void optimize_self() override; + void optimize_self(OptimizePass pass) override; void fetchPostings(const ExecuteInfo &execInfo) override; std::unique_ptr<SameElementSearch> create_same_element_search(search::fef::TermFieldMatchData& tfmd, bool strict) const; diff --git a/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp b/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp index 7dea2e621e1..e6de5449b60 100644 --- a/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp +++ b/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp @@ -7,6 +7,7 @@ #include <vespa/document/bucket/fixed_bucket_spaces.h> #include <vespa/vespalib/util/backtrace.h> #include <cassert> +#include <cinttypes> #include <vespa/log/log.h> LOG_SETUP(".distributor.distributor_bucket_space_repo"); @@ -36,7 +37,7 @@ DistributorBucketSpaceRepo::get(BucketSpace bucketSpace) { auto itr = _map.find(bucketSpace); if (itr == _map.end()) [[unlikely]] { - LOG(error, "Bucket space %zu does not have a valid mapping. %s", bucketSpace.getId(), vespalib::getStackTrace(0).c_str()); + LOG(error, "Bucket space %" PRIu64 " does not have a valid mapping. %s", bucketSpace.getId(), vespalib::getStackTrace(0).c_str()); abort(); } return *itr->second; @@ -47,7 +48,7 @@ DistributorBucketSpaceRepo::get(BucketSpace bucketSpace) const { auto itr = _map.find(bucketSpace); if (itr == _map.end()) [[unlikely]] { - LOG(error, "Bucket space %zu does not have a valid mapping. %s", bucketSpace.getId(), vespalib::getStackTrace(0).c_str()); + LOG(error, "Bucket space %" PRIu64 " does not have a valid mapping. %s", bucketSpace.getId(), vespalib::getStackTrace(0).c_str()); abort(); } return *itr->second; diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp index 8e962a59809..a69f9e55afb 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.cpp +++ b/storage/src/vespa/storage/persistence/asynchandler.cpp @@ -319,7 +319,7 @@ AsyncHandler::handle_delete_bucket_throttling(api::DeleteBucketCommand& cmd, Mes LOG(spam, "%s: completed removeByGidAsync operation", bucket.toString().c_str()); // Nothing else clever to do here. Throttle token and deleteBucket dispatch refs dropped implicitly. }); - LOG(spam, "%s: about to invoke removeByGidAsync(%s, %s, %zu)", cmd.getBucket().toString().c_str(), + LOG(spam, "%s: about to invoke removeByGidAsync(%s, %s, %" PRIu64 ")", cmd.getBucket().toString().c_str(), vespalib::string(meta->getDocumentType()).c_str(), meta->getGid().toString().c_str(), meta->getTimestamp().getValue()); _spi.removeByGidAsync(spi_bucket, std::move(to_remove), std::make_unique<ResultTaskOperationDone>(_sequencedExecutor, cmd.getBucketId(), std::move(task))); } diff --git a/vespalib/src/tests/btree/btree_test.cpp b/vespalib/src/tests/btree/btree_test.cpp index f6fa962fa9d..21838676906 100644 --- a/vespalib/src/tests/btree/btree_test.cpp +++ b/vespalib/src/tests/btree/btree_test.cpp @@ -231,6 +231,7 @@ protected: template <typename TreeType> void requireThatUpperBoundWorksT(); void requireThatIteratorDistanceWorks(int numEntries); + void test_step_forward(int num_entries); }; template <typename LeafNodeType> @@ -1519,6 +1520,33 @@ BTreeTest::requireThatIteratorDistanceWorks(int numEntries) } } +void +BTreeTest::test_step_forward(int num_entries) +{ + GenerationHandler g; + MyTree tree; + for (int i = 0; i < num_entries; ++i) { + tree.insert(i, toStr(i)); + } + auto it = tree.begin(); + auto ite = it; + ite.end(); + for (int i = 0; i <= num_entries; ++i) { + auto iit = tree.lowerBound(i); + auto iit2 = iit; + iit2 += (num_entries - i); + EXPECT_TRUE(iit2.identical(ite)); + iit2 = iit; + iit2 += (1000000 + num_entries); + EXPECT_TRUE(iit2.identical(ite)); + for (int j = i; j <= num_entries; ++j) { + auto jit = tree.lowerBound(j); + auto iit3 = iit; + iit3 += (j - i); + EXPECT_TRUE(iit3.identical(jit)); + } + } +} TEST_F(BTreeTest, require_that_iterator_distance_works) { @@ -1530,6 +1558,16 @@ TEST_F(BTreeTest, require_that_iterator_distance_works) requireThatIteratorDistanceWorks(400); } +TEST_F(BTreeTest, require_that_step_forward_works) +{ + test_step_forward(1); + test_step_forward(3); + test_step_forward(8); + test_step_forward(20); + test_step_forward(100); + test_step_forward(400); +} + TEST_F(BTreeTest, require_that_foreach_key_works) { using Tree = BTree<int, int, btree::NoAggregated, MyComp, MyTraits>; diff --git a/vespalib/src/vespa/vespalib/btree/btreeiterator.h b/vespalib/src/vespa/vespalib/btree/btreeiterator.h index 53e4d004981..9519951f9e2 100644 --- a/vespalib/src/vespa/vespalib/btree/btreeiterator.h +++ b/vespalib/src/vespa/vespalib/btree/btreeiterator.h @@ -232,6 +232,11 @@ protected: return *this; } + /* + * Step iterator forwards the given number of steps. + */ + void step_forward(size_t steps); + ~BTreeIteratorBase(); BTreeIteratorBase(const BTreeIteratorBase &other); BTreeIteratorBase &operator=(const BTreeIteratorBase &other); @@ -502,6 +507,7 @@ protected: using ParentType::_compatLeafNode; using ParentType::clearPath; using ParentType::setupEmpty; + using ParentType::step_forward; public: using ParentType::end; @@ -557,6 +563,13 @@ public: return *this; } + /* + * Step iterator forwards the given number of steps. + */ + BTreeConstIterator & operator+=(size_t steps) { + step_forward(steps); + return *this; + } /** * Position iterator at first position with a key that is greater * than or equal to the key argument. The iterator must be set up @@ -699,6 +712,7 @@ public: using ParentType::_leafRoot; using ParentType::_compatLeafNode; using ParentType::end; + using ParentType::step_forward; using EntryRef = datastore::EntryRef; BTreeIterator(BTreeNode::Ref root, const NodeAllocatorType &allocator) @@ -727,6 +741,11 @@ public: return *this; } + BTreeIterator & operator+=(size_t steps) { + step_forward(steps); + return *this; + } + NodeAllocatorType & getAllocator() const { return const_cast<NodeAllocatorType &>(*_allocator); } diff --git a/vespalib/src/vespa/vespalib/btree/btreeiterator.hpp b/vespalib/src/vespa/vespalib/btree/btreeiterator.hpp index fdab3a94a5b..3119d05cfd9 100644 --- a/vespalib/src/vespa/vespalib/btree/btreeiterator.hpp +++ b/vespalib/src/vespa/vespalib/btree/btreeiterator.hpp @@ -526,6 +526,85 @@ identical(const BTreeIteratorBase &rhs) const } +template <typename KeyT, typename DataT, typename AggrT, + uint32_t INTERNAL_SLOTS, uint32_t LEAF_SLOTS, uint32_t PATH_SIZE> +void +BTreeIteratorBase<KeyT, DataT, AggrT, INTERNAL_SLOTS, LEAF_SLOTS, PATH_SIZE>:: +step_forward(size_t steps) +{ + auto lnode = _leaf.getNode(); + if (lnode == nullptr) { + return; + } + auto idx = _leaf.getIdx(); + if (idx + steps < lnode->validSlots()) { + _leaf.setIdx(idx + steps); + return; + } + if (_pathSize == 0) { + _leaf.invalidate(); + return; + } + size_t remaining_steps = steps - (lnode->validSlots() - idx); + uint32_t level = 0; + uint32_t levels = _pathSize; + const InternalNodeType* node; + /* + * Find intermediate node representing subtree containing old and new + * position. + */ + for (;;) { + node = _path[level].getNode(); + idx = _path[level].getIdx() + 1; + while (idx < node->validSlots()) { + auto ref = node->getChild(idx); + auto valid_leaves = (level != 0) ? + _allocator->mapInternalRef(ref)->validLeaves() : + _allocator->mapLeafRef(ref)->validLeaves(); + if (remaining_steps < valid_leaves) { + break; + } + remaining_steps -= valid_leaves; + ++idx; + } + if (idx < node->validSlots()) { + break; + } else { + ++level; + if (level == levels) { + end(); + return; + } + } + } + /* + * Walk down subtree adjusting iterator for new position. + */ + _path[level].setIdx(idx); + while (level > 0) { + --level; + node = _allocator->mapInternalRef(node->getChild(idx)); + assert(remaining_steps < node->validLeaves()); + idx = 0; + while (idx < node->validSlots()) { + auto ref = node->getChild(idx); + auto valid_leaves = (level != 0) ? + _allocator->mapInternalRef(ref)->validLeaves() : + _allocator->mapLeafRef(ref)->validLeaves(); + if (remaining_steps < valid_leaves) { + break; + } + remaining_steps -= valid_leaves; + ++idx; + } + assert(idx < node->validSlots()); + _path[level].setNodeAndIdx(node, idx); + } + lnode = _allocator->mapLeafRef(node->getChild(idx)); + assert(remaining_steps < lnode->validSlots()); + _leaf.setNodeAndIdx(lnode, remaining_steps); +} + template <typename KeyT, typename DataT, typename AggrT, typename CompareT, typename TraitsT> void |