diff options
92 files changed, 908 insertions, 989 deletions
diff --git a/client/CMakeLists.txt b/client/CMakeLists.txt index 07361e4b2eb..9ab262cffb9 100644 --- a/client/CMakeLists.txt +++ b/client/CMakeLists.txt @@ -11,6 +11,6 @@ add_custom_command(OUTPUT ${GODIR}/bin/vespa-logfmt ${GODIR}/bin/vespa-deploy add_custom_target(vespalog_logfmt ALL DEPENDS ${GODIR}/bin/vespa-logfmt) -install(PROGRAMS ${GODIR}/bin/vespa-logfmt DESTINATION bin) -install(PROGRAMS ${GODIR}/bin/vespa-deploy DESTINATION bin) install(PROGRAMS ${GODIR}/bin/script-utils DESTINATION libexec/vespa) +install_symlink(libexec/vespa/script-utils bin/vespa-logfmt) +install_symlink(libexec/vespa/script-utils bin/vespa-deploy) diff --git a/client/go/vespa-deploy/cmd.go b/client/go/cmd/deploy/cmd.go index af97dc098e5..f5cf554834d 100644 --- a/client/go/vespa-deploy/cmd.go +++ b/client/go/cmd/deploy/cmd.go @@ -2,7 +2,7 @@ // vespa-deploy command // Author: arnej -package main +package deploy import ( "fmt" @@ -10,7 +10,6 @@ import ( "github.com/spf13/cobra" "github.com/vespa-engine/vespa/client/go/build" - "github.com/vespa-engine/vespa/client/go/cmd/deploy" ) func reallySimpleHelp(cmd *cobra.Command, args []string) { @@ -19,7 +18,7 @@ func reallySimpleHelp(cmd *cobra.Command, args []string) { func NewDeployCmd() *cobra.Command { var ( - curOptions deploy.Options + curOptions Options ) cobra.EnableCommandSorting = false cmd := &cobra.Command{ @@ -64,15 +63,15 @@ Try 'vespa-deploy help <command>' to get more help`, return cmd } -func newUploadCmd(opts *deploy.Options) *cobra.Command { +func newUploadCmd(opts *Options) *cobra.Command { cmd := &cobra.Command{ Use: "upload <application package>", Run: func(cmd *cobra.Command, args []string) { - opts.Command = deploy.CmdUpload + opts.Command = CmdUpload if opts.Verbose { fmt.Printf("upload %v [%v]\n", opts, args) } - err := deploy.RunUpload(opts, args) + err := RunUpload(opts, args) if err != nil { fmt.Fprintf(os.Stderr, "%s\n", err.Error()) os.Exit(1) @@ -85,15 +84,15 @@ func newUploadCmd(opts *deploy.Options) *cobra.Command { return cmd } -func newPrepareCmd(opts *deploy.Options) *cobra.Command { +func newPrepareCmd(opts *Options) *cobra.Command { cmd := &cobra.Command{ Use: "prepare [<session_id> | <application package>]", Run: func(cmd *cobra.Command, args []string) { - opts.Command = deploy.CmdPrepare + opts.Command = CmdPrepare if opts.Verbose { fmt.Printf("prepare %v [%v]\n", opts, args) } - err := deploy.RunPrepare(opts, args) + err := RunPrepare(opts, args) if err != nil { fmt.Fprintf(os.Stderr, "%s\n", err.Error()) os.Exit(1) @@ -105,15 +104,15 @@ func newPrepareCmd(opts *deploy.Options) *cobra.Command { return cmd } -func newActivateCmd(opts *deploy.Options) *cobra.Command { +func newActivateCmd(opts *Options) *cobra.Command { cmd := &cobra.Command{ Use: "activate [<session_id>]", Run: func(cmd *cobra.Command, args []string) { - opts.Command = deploy.CmdActivate + opts.Command = CmdActivate if opts.Verbose { fmt.Printf("activate %v [%v]\n", opts, args) } - err := deploy.RunActivate(opts, args) + err := RunActivate(opts, args) if err != nil { fmt.Fprintf(os.Stderr, "%s\n", err.Error()) os.Exit(1) @@ -125,15 +124,15 @@ func newActivateCmd(opts *deploy.Options) *cobra.Command { return cmd } -func newFetchCmd(opts *deploy.Options) *cobra.Command { +func newFetchCmd(opts *Options) *cobra.Command { cmd := &cobra.Command{ Use: "fetch <output directory>", Run: func(cmd *cobra.Command, args []string) { - opts.Command = deploy.CmdFetch + opts.Command = CmdFetch if opts.Verbose { fmt.Printf("fetch %v [%v]\n", opts, args) } - err := deploy.RunFetch(opts, args) + err := RunFetch(opts, args) if err != nil { fmt.Fprintf(os.Stderr, "%s\n", err.Error()) os.Exit(1) diff --git a/client/go/vespa-logfmt/cmd.go b/client/go/cmd/logfmt/cmd.go index c36f92304f1..54e82844a30 100644 --- a/client/go/vespa-logfmt/cmd.go +++ b/client/go/cmd/logfmt/cmd.go @@ -2,17 +2,16 @@ // vespa-logfmt command // Author: arnej -package main +package logfmt import ( "github.com/spf13/cobra" "github.com/vespa-engine/vespa/client/go/build" - "github.com/vespa-engine/vespa/client/go/cmd/logfmt" ) func NewLogfmtCmd() *cobra.Command { var ( - curOptions logfmt.Options = logfmt.NewOptions() + curOptions Options = NewOptions() ) cmd := &cobra.Command{ Use: "vespa-logfmt", @@ -21,7 +20,7 @@ func NewLogfmtCmd() *cobra.Command { and converts it to something human-readable`, Version: build.Version, Run: func(cmd *cobra.Command, args []string) { - logfmt.RunLogfmt(&curOptions, args) + RunLogfmt(&curOptions, args) }, } cmd.Flags().VarP(&curOptions.ShowLevels, "level", "l", "turn levels on/off\n") diff --git a/client/go/script-utils/main.go b/client/go/script-utils/main.go index a7160691a5d..7ab16a6f831 100644 --- a/client/go/script-utils/main.go +++ b/client/go/script-utils/main.go @@ -6,16 +6,25 @@ package main import ( "fmt" "os" + "strings" + "github.com/vespa-engine/vespa/client/go/cmd/deploy" + "github.com/vespa-engine/vespa/client/go/cmd/logfmt" "github.com/vespa-engine/vespa/client/go/vespa" ) +func basename(s string) string { + parts := strings.Split(s, "/") + return parts[len(parts)-1] +} + func main() { - if len(os.Args) < 2 { - fmt.Fprintln(os.Stderr, "actions: export-env, ipv6-only") - return + action := basename(os.Args[0]) + if action == "script-utils" && len(os.Args) > 1 { + action = os.Args[1] + os.Args = os.Args[1:] } - switch os.Args[1] { + switch action { case "export-env": vespa.ExportDefaultEnvToSh() case "ipv6-only": @@ -24,7 +33,16 @@ func main() { } else { os.Exit(1) } + case "vespa-deploy": + _ = vespa.FindHome() + cobra := deploy.NewDeployCmd() + cobra.Execute() + case "vespa-logfmt": + _ = vespa.FindHome() + cobra := logfmt.NewLogfmtCmd() + cobra.Execute() default: - fmt.Fprintf(os.Stderr, "unknown action '%s'\n", os.Args[1]) + fmt.Fprintf(os.Stderr, "unknown action '%s'\n", action) + fmt.Fprintln(os.Stderr, "actions: export-env, ipv6-only, vespa-deploy, vespa-logfmt") } } diff --git a/client/go/vespa-deploy/main.go b/client/go/vespa-deploy/main.go deleted file mode 100644 index 549f5511765..00000000000 --- a/client/go/vespa-deploy/main.go +++ /dev/null @@ -1,10 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -// vespa-deploy command -// Author: arnej - -package main - -func main() { - cobra := NewDeployCmd() - cobra.Execute() -} diff --git a/client/go/vespa-logfmt/main.go b/client/go/vespa-logfmt/main.go deleted file mode 100644 index 90a7719a1ef..00000000000 --- a/client/go/vespa-logfmt/main.go +++ /dev/null @@ -1,9 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -// Author: arnej - -package main - -func main() { - cobra := NewLogfmtCmd() - cobra.Execute() -} diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java index 746432f1d38..3f1a7ab5d7b 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java @@ -98,9 +98,6 @@ abstract public class NodeInfo implements Comparable<NodeInfo> { private int prematureCrashCount = 0; - /** Remember last time we adjusted version, such that if we had multiple requests pending when we did, we can avoid printing error right after. */ - private long adjustedVersionTime = 0; - private HostInfo hostInfo = HostInfo.createHostInfo("{}"); private Group group; @@ -382,29 +379,6 @@ abstract public class NodeInfo implements Comparable<NodeInfo> { return nextAttemptTime; } - /** @return True if we demoted communication version so this can be valid error. */ - public boolean notifyNoSuchMethodError(String methodName, Timer timer) { - if (methodName.equals(RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME)) { - if (version == RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_VERSION) { - downgradeToRpcVersion(RPCCommunicator.LEGACY_SET_SYSTEM_STATE2_RPC_VERSION, methodName, timer); - return true; - } else if (timer.getCurrentTimeInMillis() - 2000 < adjustedVersionTime) { - log.log(Level.FINE, () -> "Node " + toString() + " does not support " + methodName + " call. Version already downgraded, so ignoring it."); - return true; - } - } - log.log(Level.WARNING, "Node " + toString() + " does not support " + methodName + " which it should."); - return false; - } - - private void downgradeToRpcVersion(int newVersion, String methodName, Timer timer) { - log.log(Level.FINE, () -> String.format("Node %s does not support %s call. Downgrading to version %d.", - toString(), methodName, newVersion)); - version = newVersion; - nextAttemptTime = 0; - adjustedVersionTime = timer.getCurrentTimeInMillis(); - } - public Target getConnection() { return connection; } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java index 3738a17f4b6..58bbd03044f 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java @@ -37,15 +37,18 @@ public class RPCCommunicator implements Communicator { public static final Logger log = Logger.getLogger(RPCCommunicator.class.getName()); + // Rpc method versions and Vespa versions which supports each version: + // 0 - 4.1 + // 1 - 4.2-5.0.10 + // 2 - 5.0.11-8.48.3 + // 3 - 6.220+ + // 4 - 7.24+ public static final int ACTIVATE_CLUSTER_STATE_VERSION_RPC_VERSION = 4; public static final String ACTIVATE_CLUSTER_STATE_VERSION_RPC_METHOD_NAME = "activate_cluster_state_version"; public static final int SET_DISTRIBUTION_STATES_RPC_VERSION = 3; public static final String SET_DISTRIBUTION_STATES_RPC_METHOD_NAME = "setdistributionstates"; - public static final int LEGACY_SET_SYSTEM_STATE2_RPC_VERSION = 2; - public static final String LEGACY_SET_SYSTEM_STATE2_RPC_METHOD_NAME = "setsystemstate2"; - private final Timer timer; private final Supervisor supervisor; private Duration nodeStateRequestTimeoutIntervalMax; @@ -121,7 +124,7 @@ public class RPCCommunicator implements Communicator { req.parameters().add(new Int32Value(fleetControllerIndex)); RPCGetNodeStateRequest stateRequest = new RPCGetNodeStateRequest(node, req); - RPCGetNodeStateWaiter waiter = new RPCGetNodeStateWaiter(stateRequest, externalWaiter, timer); + RPCGetNodeStateWaiter waiter = new RPCGetNodeStateWaiter(stateRequest, externalWaiter); Duration requestTimeout = nodeStateRequestTimeoutIntervalMax.plus(nodeStateRequestRoundTripTimeMax); @@ -140,20 +143,13 @@ public class RPCCommunicator implements Communicator { log.log(Level.FINE, () -> String.format("Connection to '%s' could not be created.", node.getRpcAddress())); return; } - int nodeVersion = node.getVersion(); - Request req; - if (nodeVersion <= 2) { - req = new Request(LEGACY_SET_SYSTEM_STATE2_RPC_METHOD_NAME); - req.parameters().add(new StringValue(baselineState.toString(false))); - } else { - req = new Request(SET_DISTRIBUTION_STATES_RPC_METHOD_NAME); - SlimeClusterStateBundleCodec codec = new SlimeClusterStateBundleCodec(); - EncodedClusterStateBundle encodedBundle = codec.encode(stateBundle); - Values v = req.parameters(); - v.add(new Int8Value(encodedBundle.getCompression().type().getCode())); - v.add(new Int32Value(encodedBundle.getCompression().uncompressedSize())); - v.add(new DataValue(encodedBundle.getCompression().data())); - } + Request req = new Request(SET_DISTRIBUTION_STATES_RPC_METHOD_NAME); + SlimeClusterStateBundleCodec codec = new SlimeClusterStateBundleCodec(); + EncodedClusterStateBundle encodedBundle = codec.encode(stateBundle); + Values v = req.parameters(); + v.add(new Int8Value(encodedBundle.getCompression().type().getCode())); + v.add(new Int32Value(encodedBundle.getCompression().uncompressedSize())); + v.add(new DataValue(encodedBundle.getCompression().data())); log.log(Level.FINE, () -> String.format("Sending '%s' RPC to %s for state version %d", req.methodName(), node.getRpcAddress(), stateBundle.getVersion())); diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCGetNodeStateWaiter.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCGetNodeStateWaiter.java index 5f77e831918..74ce1668b2b 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCGetNodeStateWaiter.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCGetNodeStateWaiter.java @@ -6,7 +6,6 @@ import com.yahoo.jrt.Request; import com.yahoo.jrt.RequestWaiter; import com.yahoo.vespa.clustercontroller.core.Communicator; import com.yahoo.vespa.clustercontroller.core.GetNodeStateRequest; -import com.yahoo.vespa.clustercontroller.core.Timer; /** * Handles the reply to a get node state request to a node. @@ -15,23 +14,13 @@ public class RPCGetNodeStateWaiter implements RequestWaiter { private final RPCGetNodeStateRequest request; private final Communicator.Waiter<GetNodeStateRequest> waiter; - private final Timer timer; - public RPCGetNodeStateWaiter(RPCGetNodeStateRequest request, - Communicator.Waiter<GetNodeStateRequest> waiter, Timer timer) { + public RPCGetNodeStateWaiter(RPCGetNodeStateRequest request, Communicator.Waiter<GetNodeStateRequest> waiter) { this.request = request; this.waiter = waiter; - this.timer = timer; } private GetNodeStateRequest.Reply convertToReply(Request req) { - if (req.errorCode() == ErrorCode.NO_SUCH_METHOD) { - // If we get no such method error, and we downgrade version, we must retry. May be ok that it doesn't exist - if (request.getNodeInfo().notifyNoSuchMethodError(req.methodName(), timer)) { - return new GetNodeStateRequest.Reply(Communicator.TRANSIENT_ERROR, "Downgrading version"); - } - } - if (req.isError()) { return new GetNodeStateRequest.Reply(req.errorCode(), req.errorMessage()); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCSetClusterStateWaiter.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCSetClusterStateWaiter.java index 89f37173701..41fc7f67fbb 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCSetClusterStateWaiter.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCSetClusterStateWaiter.java @@ -30,13 +30,7 @@ public class RPCSetClusterStateWaiter implements RequestWaiter { public SetClusterStateRequest.Reply getReply(Request req) { NodeInfo info = request.getNodeInfo(); - if (req.methodName().equals(RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME) - || req.methodName().equals(RPCCommunicator.LEGACY_SET_SYSTEM_STATE2_RPC_METHOD_NAME)) { - if (req.isError() && req.errorCode() == ErrorCode.NO_SUCH_METHOD) { - if (info.notifyNoSuchMethodError(req.methodName(), timer)) { - return new SetClusterStateRequest.Reply(Communicator.TRANSIENT_ERROR, "Trying lower version"); - } - } + if (req.methodName().equals(RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME)) { if (req.isError()) { return new SetClusterStateRequest.Reply(req.errorCode(), req.errorMessage()); } else if (!req.checkReturnTypes("")) { diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DistributionBitCountTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DistributionBitCountTest.java index 0f2e2e23492..f7e376c419d 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DistributionBitCountTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DistributionBitCountTest.java @@ -26,7 +26,7 @@ public class DistributionBitCountTest extends FleetControllerTest { builder.setDistributionBits(17); setUpFleetController(false, builder); startingTest(testName); - List<DummyVdsNode> nodes = setUpVdsNodes(false, new DummyVdsNodeOptions(), true, configuredNodes); + List<DummyVdsNode> nodes = setUpVdsNodes(false, true, configuredNodes); for (DummyVdsNode node : nodes) { node.setNodeState(new NodeState(node.getType(), State.UP).setMinUsedBits(20)); node.connect(); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNode.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNode.java index fbaf41fdd47..5981c2a1cf6 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNode.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNode.java @@ -33,8 +33,6 @@ import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; -import static ai.vespa.validation.Validation.requireAtLeast; - /** * * Used to fake a node in VDS, such that we can test the fleetcontroller without dummy interface for talking to @@ -53,8 +51,6 @@ public class DummyVdsNode { private Supervisor supervisor; private Acceptor acceptor; private Register register; - private final int stateCommunicationVersion; - private boolean negotiatedHandle = false; private final Timer timer; private boolean failSetSystemStateRequests = false; private boolean resetTimestampOnReconnect = false; @@ -120,7 +116,7 @@ public class DummyVdsNode { } }; - public DummyVdsNode(Timer timer, DummyVdsNodeOptions options, String[] slobrokConnectionSpecs, String clusterName, + public DummyVdsNode(Timer timer, String[] slobrokConnectionSpecs, String clusterName, NodeType nodeType, int index) throws Exception { this.timer = timer; this.slobrokConnectionSpecs = slobrokConnectionSpecs; @@ -128,7 +124,6 @@ public class DummyVdsNode { type = nodeType; this.index = index; this.nodeState = new NodeState(type, State.UP); - this.stateCommunicationVersion = requireAtLeast(options.stateCommunicationVersion, "state communication version cannot be less than 2", 2); messageResponder.start(); nodeState.setStartTimestamp(timer.getCurrentTimeInMillis() / 1000); } @@ -154,7 +149,6 @@ public class DummyVdsNode { slist.setup(slobrokConnectionSpecs); register = new Register(supervisor, slist, new Spec("localhost", acceptor.port()), new BackOff()); registerSlobrok(); - negotiatedHandle = false; return acceptor.port(); } @@ -197,8 +191,6 @@ public class DummyVdsNode { return new Node(type, index); } - public int getStateCommunicationVersion() { return stateCommunicationVersion; } - void waitForSystemStateVersion(int version, Duration timeout) { try { Instant endTime = Instant.now().plus(timeout); @@ -302,58 +294,32 @@ public class DummyVdsNode { m.returnDesc(0, "returnCode", "Returncode of request. Should be 0 = OK"); supervisor.addMethod(m); - m = new Method("setsystemstate", "s", "is", this::rpc_setSystemState); - m.methodDesc("Set system state of entire system"); - m.paramDesc(0, "systemState", "new systemstate"); - m.returnDesc(0, "returnCode", "Returncode of request. Should be 1 = OK"); - m.returnDesc(1, "returnMessage", "Textual error message if returncode is not ok."); + m = new Method("getnodestate3", "sii", "ss", this::rpc_getNodeState2); + m.methodDesc("Get nodeState of a node, answer when state changes from given state."); + m.paramDesc(0, "nodeStateIn", "The node state of the given node"); + m.paramDesc(1, "timeout", "Time timeout in milliseconds set by the state requester."); + m.returnDesc(0, "nodeStateOut", "The node state of the given node"); + m.returnDesc(1, "hostinfo", "Information on the host node is running on"); supervisor.addMethod(m); - if (stateCommunicationVersion > 0) { - m = new Method("getnodestate2", "si", "s", this::rpc_getNodeState2); - m.methodDesc("Get nodeState of a node, answer when state changes from given state."); - m.paramDesc(0, "nodeStateIn", "The node state of the given node"); - m.paramDesc(1, "timeout", "Time timeout in milliseconds set by the state requester."); - m.returnDesc(0, "nodeStateOut", "The node state of the given node"); - supervisor.addMethod(m); - - m = new Method("setsystemstate2", "s", "", this::rpc_setSystemState2); - m.methodDesc("Set system state of entire system"); - m.paramDesc(0, "systemState", "new systemstate"); - supervisor.addMethod(m); - - if (stateCommunicationVersion > 1) { - m = new Method("getnodestate3", "sii", "ss", this::rpc_getNodeState2); - m.methodDesc("Get nodeState of a node, answer when state changes from given state."); - m.paramDesc(0, "nodeStateIn", "The node state of the given node"); - m.paramDesc(1, "timeout", "Time timeout in milliseconds set by the state requester."); - m.returnDesc(0, "nodeStateOut", "The node state of the given node"); - m.returnDesc(1, "hostinfo", "Information on the host node is running on"); - supervisor.addMethod(m); - } - } - if (stateCommunicationVersion >= RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_VERSION) { - m = new Method(RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME, "bix", "", this::rpc_setDistributionStates); - m.methodDesc("Set distribution states for cluster and bucket spaces"); - m.paramDesc(0, "compressionType", "Compression type for payload"); - m.paramDesc(1, "uncompressedSize", "Uncompressed size of payload"); - m.paramDesc(2, "payload", "Slime format payload"); - supervisor.addMethod(m); - } - if (stateCommunicationVersion >= RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_VERSION) { - m = new Method(RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_METHOD_NAME, "i", "i", this::rpc_activateClusterStateVersion); - m.methodDesc("Activate a given cluster state version"); - m.paramDesc(0, "stateVersion", "Cluster state version to activate"); - m.returnDesc(0, "actualVersion", "Actual cluster state version on node"); - supervisor.addMethod(m); - } + m = new Method(RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME, "bix", "", this::rpc_setDistributionStates); + m.methodDesc("Set distribution states for cluster and bucket spaces"); + m.paramDesc(0, "compressionType", "Compression type for payload"); + m.paramDesc(1, "uncompressedSize", "Uncompressed size of payload"); + m.paramDesc(2, "payload", "Slime format payload"); + supervisor.addMethod(m); + + m = new Method(RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_METHOD_NAME, "i", "i", this::rpc_activateClusterStateVersion); + m.methodDesc("Activate a given cluster state version"); + m.paramDesc(0, "stateVersion", "Cluster state version to activate"); + m.returnDesc(0, "actualVersion", "Actual cluster state version on node"); + supervisor.addMethod(m); } private void rpc_storageConnect(Request req) { synchronized(timer) { log.log(Level.FINEST, () -> "Dummy node " + this + " got old type handle connect message."); req.returnValues().add(new Int32Value(0)); - negotiatedHandle = true; } } @@ -437,56 +403,6 @@ public class DummyVdsNode { } } - private void rpc_setSystemState(Request req) { - try{ - if (shouldFailSetSystemStateRequests()) { - req.setError(ErrorCode.GENERAL_ERROR, "Dummy node configured to fail setSystemState() calls"); - return; - } - if (!negotiatedHandle) { - req.setError(75000, "Connection not bound to a handle"); - return; - } - ClusterState newState = new ClusterState(req.parameters().get(0).asString()); - synchronized(timer) { - updateStartTimestamps(newState); - clusterStateBundles.add(0, ClusterStateBundle.ofBaselineOnly(AnnotatedClusterState.withoutAnnotations(newState))); - timer.notifyAll(); - } - req.returnValues().add(new Int32Value(1)); - req.returnValues().add(new StringValue("OK")); - log.log(Level.FINE, () -> "Dummy node " + this + ": Got new system state (through old setsystemstate call) " + newState); - } catch (Exception e) { - log.log(Level.SEVERE, "Dummy node " + this + ": An error occurred when answering setsystemstate request: " + e.getMessage()); - e.printStackTrace(System.err); - req.returnValues().add(new Int32Value(ErrorCode.METHOD_FAILED)); - req.returnValues().add(new StringValue(e.getMessage())); - } - } - - private void rpc_setSystemState2(Request req) { - try{ - if (shouldFailSetSystemStateRequests()) { - req.setError(ErrorCode.GENERAL_ERROR, "Dummy node configured to fail setSystemState2() calls"); - return; - } - ClusterState newState = new ClusterState(req.parameters().get(0).asString()); - synchronized(timer) { - updateStartTimestamps(newState); - clusterStateBundles.add(0, ClusterStateBundle.ofBaselineOnly(AnnotatedClusterState.withoutAnnotations(newState))); - if (stateCommunicationVersion < RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_VERSION) { - activatedClusterStateVersion = newState.getVersion(); // Simulate node that does not know of activation - } - timer.notifyAll(); - } - log.log(Level.FINE, () -> "Dummy node " + this + ": Got new system state " + newState); - } catch (Exception e) { - log.log(Level.SEVERE, "Dummy node " + this + ": An error occurred when answering setsystemstate request: " + e.getMessage()); - e.printStackTrace(System.err); - req.setError(ErrorCode.METHOD_FAILED, e.getMessage()); - } - } - private void rpc_setDistributionStates(Request req) { try { if (shouldFailSetSystemStateRequests()) { @@ -497,9 +413,6 @@ public class DummyVdsNode { synchronized(timer) { updateStartTimestamps(stateBundle.getBaselineClusterState()); clusterStateBundles.add(0, stateBundle); - if (stateCommunicationVersion < RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_VERSION) { - activatedClusterStateVersion = stateBundle.getVersion(); // Simulate node that does not know of activation - } timer.notifyAll(); } log.log(Level.FINE, () -> "Dummy node " + this + ": Got new cluster state " + stateBundle); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNodeOptions.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNodeOptions.java deleted file mode 100644 index d6b5e7fa5d2..00000000000 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DummyVdsNodeOptions.java +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.clustercontroller.core; - -import com.yahoo.vespa.clustercontroller.core.rpc.RPCCommunicator; - -/** - * Options for simulated content nodes that are registered in Slobrok and communicate - * over regular RPC. - */ -public class DummyVdsNodeOptions { - // Rpc method versions and which Vespa versions supports each version: - // 0 - 4.1 - // 1 - 4.2-5.0.10 - // 2 - 5.0.11+ - // 3 - 6.220+ - // 4 - 7.24+ - int stateCommunicationVersion = RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_VERSION; -} diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java index bf06474a3af..2a77b4c7636 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java @@ -173,28 +173,28 @@ public abstract class FleetControllerTest implements Waiter { } protected void setUpVdsNodes(boolean useFakeTimer) throws Exception { - setUpVdsNodes(useFakeTimer, new DummyVdsNodeOptions(), false); + setUpVdsNodes(useFakeTimer, false); } - protected void setUpVdsNodes(boolean useFakeTimer, DummyVdsNodeOptions options, boolean startDisconnected) throws Exception { - setUpVdsNodes(useFakeTimer, options, startDisconnected, DEFAULT_NODE_COUNT); + protected void setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected) throws Exception { + setUpVdsNodes(useFakeTimer, startDisconnected, DEFAULT_NODE_COUNT); } - protected void setUpVdsNodes(boolean useFakeTimer, DummyVdsNodeOptions options, boolean startDisconnected, int nodeCount) throws Exception { + protected void setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected, int nodeCount) throws Exception { TreeSet<Integer> nodeIndexes = new TreeSet<>(); for (int i = 0; i < nodeCount; ++i) nodeIndexes.add(this.nodes.size()/2 + i); // divide by 2 because there are 2 nodes (storage and distributor) per index - setUpVdsNodes(useFakeTimer, options, startDisconnected, nodeIndexes); + setUpVdsNodes(useFakeTimer, startDisconnected, nodeIndexes); } - protected void setUpVdsNodes(boolean useFakeTimer, DummyVdsNodeOptions nodeOptions, boolean startDisconnected, Set<Integer> nodeIndexes) throws Exception { + protected void setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected, Set<Integer> nodeIndexes) throws Exception { for (int nodeIndex : nodeIndexes) { - nodes.add(createNode(useFakeTimer, nodeOptions, startDisconnected, DISTRIBUTOR, nodeIndex)); - nodes.add(createNode(useFakeTimer, nodeOptions, startDisconnected, STORAGE, nodeIndex)); + nodes.add(createNode(useFakeTimer, startDisconnected, DISTRIBUTOR, nodeIndex)); + nodes.add(createNode(useFakeTimer, startDisconnected, STORAGE, nodeIndex)); } } - private DummyVdsNode createNode(boolean useFakeTimer, DummyVdsNodeOptions nodeOptions, boolean startDisconnected, + private DummyVdsNode createNode(boolean useFakeTimer, boolean startDisconnected, NodeType nodeType, int nodeIndex) throws Exception { String[] connectionSpecs = getSlobrokConnectionSpecs(slobrok); - DummyVdsNode node = new DummyVdsNode(useFakeTimer ? timer : new RealTimer(), nodeOptions, connectionSpecs, + DummyVdsNode node = new DummyVdsNode(useFakeTimer ? timer : new RealTimer(), connectionSpecs, options.clusterName(), nodeType, nodeIndex); if ( ! startDisconnected) node.connect(); @@ -208,11 +208,11 @@ public abstract class FleetControllerTest implements Waiter { * As two dummy nodes are created for each configured node - one distributor and one storage node - * the returned list is twice as large as configuredNodes. */ - protected List<DummyVdsNode> setUpVdsNodes(boolean useFakeTimer, DummyVdsNodeOptions options, boolean startDisconnected, List<ConfiguredNode> configuredNodes) throws Exception { + protected List<DummyVdsNode> setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected, List<ConfiguredNode> configuredNodes) throws Exception { nodes = new ArrayList<>(); for (ConfiguredNode configuredNode : configuredNodes) { - nodes.add(createNode(useFakeTimer, options, startDisconnected, DISTRIBUTOR, configuredNode.index())); - nodes.add(createNode(useFakeTimer, options, startDisconnected, STORAGE, configuredNode.index())); + nodes.add(createNode(useFakeTimer, startDisconnected, DISTRIBUTOR, configuredNode.index())); + nodes.add(createNode(useFakeTimer, startDisconnected, STORAGE, configuredNode.index())); } return nodes; } diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/GroupAutoTakedownLiveConfigTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/GroupAutoTakedownLiveConfigTest.java index 9a5be27da94..036d3edd2a8 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/GroupAutoTakedownLiveConfigTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/GroupAutoTakedownLiveConfigTest.java @@ -42,7 +42,7 @@ public class GroupAutoTakedownLiveConfigTest extends FleetControllerTest { private FleetControllerOptions setUp3x3ClusterWithMinNodeRatio(double minNodeRatio) throws Exception { FleetControllerOptions.Builder options = createOptions(DistributionBuilder.withGroups(3).eachWithNodeCount(3), minNodeRatio); setUpFleetController(true, options); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, 9); + setUpVdsNodes(true, false, 9); waitForState("version:\\d+ distributor:9 storage:9"); return options.build(); } diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeSlobrokConfigurationMembershipTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeSlobrokConfigurationMembershipTest.java index 9acaf46b144..a760adf3cf2 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeSlobrokConfigurationMembershipTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeSlobrokConfigurationMembershipTest.java @@ -24,7 +24,7 @@ public class NodeSlobrokConfigurationMembershipTest extends FleetControllerTest setUpFleetController(true, options); Set<Integer> nodesWithStranger = new TreeSet<>(validIndices); nodesWithStranger.add(foreignNodeIndex); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, nodesWithStranger); + setUpVdsNodes(true, false, nodesWithStranger); return options.build(); } @@ -64,7 +64,7 @@ public class NodeSlobrokConfigurationMembershipTest extends FleetControllerTest Set<ConfiguredNode> configuredNodes = asConfiguredNodes(nodeIndices); FleetControllerOptions.Builder builder = optionsForConfiguredNodes(configuredNodes); options = setUpFleetController(true, builder); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, nodeIndices); + setUpVdsNodes(true, false, nodeIndices); waitForState("version:\\d+ distributor:4 storage:4"); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java index 63505bbd251..4b4cb348968 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java @@ -263,7 +263,7 @@ public class RpcServerTest extends FleetControllerTest { .setMaxInitProgressTime(30000) .setStableStateTimePeriod(60000); setUpFleetController(true, builder); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, configuredNodes); + setUpVdsNodes(true, false, configuredNodes); waitForState("version:\\d+ distributor:5 storage:5 .4.s:r"); setWantedNodeState(State.DOWN, NodeType.DISTRIBUTOR, 2); @@ -297,7 +297,7 @@ public class RpcServerTest extends FleetControllerTest { .setMaxInitProgressTime(30000) .setStableStateTimePeriod(60000); setUpFleetController(true, builder); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, configuredNodes); + setUpVdsNodes(true, false, configuredNodes); waitForState("version:\\d+ distributor:5 storage:5"); } @@ -311,7 +311,7 @@ public class RpcServerTest extends FleetControllerTest { } { // Configuration change: Add 2 new nodes and retire the 5 existing ones - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, 2); + setUpVdsNodes(true, false, 2); Set<ConfiguredNode> configuredNodes = new TreeSet<>(); for (int i = 0; i < 5; i++) configuredNodes.add(new ConfiguredNode(i, true)); @@ -379,7 +379,7 @@ public class RpcServerTest extends FleetControllerTest { .setStableStateTimePeriod(60000); options = builder.build(); setUpFleetController(true, builder); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, configuredNodes); + setUpVdsNodes(true, false, configuredNodes); waitForState("version:\\d+ distributor:5 storage:5"); } @@ -396,7 +396,7 @@ public class RpcServerTest extends FleetControllerTest { } { // Configuration change: Add 2 new nodes and retire the 5 existing ones - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, 2); + setUpVdsNodes(true, false, 2); Set<ConfiguredNode> configuredNodes = new TreeSet<>(); for (int i = 0; i < 5; i++) configuredNodes.add(new ConfiguredNode(i, true)); @@ -453,7 +453,7 @@ public class RpcServerTest extends FleetControllerTest { FleetControllerOptions.Builder options = defaultOptions("mycluster", configuredNodes); //options.setStorageDistribution(new Distribution(getDistConfig(nodeIndexes))); setUpFleetController(true, options); - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, nodeIndexes); + setUpVdsNodes(true, false, nodeIndexes); waitForState("version:\\d+ distributor:26 .0.s:d .1.s:d .2.s:d .3.s:d .5.s:d .7.s:d .8.s:d .11.s:d .12.s:d .13.s:d .15.s:d .17.s:d .18.s:d .19.s:d .20.s:d .24.s:d storage:26 .0.s:d .1.s:d .2.s:d .3.s:d .5.s:d .7.s:d .8.s:d .11.s:d .12.s:d .13.s:d .15.s:d .17.s:d .18.s:d .19.s:d .20.s:d .24.s:d"); int rpcPort = fleetController.getRpcPort(); @@ -541,7 +541,7 @@ public class RpcServerTest extends FleetControllerTest { startingTest("RpcServerTest::testGetNodeList"); setUpFleetController(true, defaultOptions("mycluster", 5)); final int nodeCount = 5; - setUpVdsNodes(true, new DummyVdsNodeOptions(), false, nodeCount); + setUpVdsNodes(true, false, nodeCount); waitForStableSystem(); assertTrue(nodes.get(0).isDistributor()); @@ -573,7 +573,7 @@ public class RpcServerTest extends FleetControllerTest { continue; } assertNotEquals("", rpc[i]); - Request req2 = new Request("getnodestate2"); + Request req2 = new Request("getnodestate3"); req2.parameters().add(new StringValue("unknown")); Target connection2 = supervisor.connect(new Spec(rpc[i])); connection2.invokeSync(req2, timeout()); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcVersionAutoDowngradeTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcVersionAutoDowngradeTest.java deleted file mode 100644 index 3740c6de491..00000000000 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcVersionAutoDowngradeTest.java +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.clustercontroller.core; - -import com.yahoo.vdslib.distribution.ConfiguredNode; -import com.yahoo.vdslib.state.NodeState; -import com.yahoo.vdslib.state.State; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; - -import java.util.ArrayList; -import java.util.List; - -@ExtendWith(CleanupZookeeperLogsOnSuccess.class) -public class RpcVersionAutoDowngradeTest extends FleetControllerTest { - - private void setUpFakeCluster(int nodeRpcVersion) throws Exception { - List<ConfiguredNode> configuredNodes = new ArrayList<>(); - for (int i = 0 ; i < 10; i++) { - configuredNodes.add(new ConfiguredNode(i, false)); - } - FleetControllerOptions.Builder options = defaultOptions("mycluster", configuredNodes); - setUpFleetController(false, options); - DummyVdsNodeOptions nodeOptions = new DummyVdsNodeOptions(); - nodeOptions.stateCommunicationVersion = nodeRpcVersion; - List<DummyVdsNode> nodes = setUpVdsNodes(false, nodeOptions, true, configuredNodes); - for (DummyVdsNode node : nodes) { - node.setNodeState(new NodeState(node.getType(), State.UP).setMinUsedBits(20)); - node.connect(); - } - } - - @Test - void cluster_state_rpc_version_is_auto_downgraded_and_retried_for_older_nodes() throws Exception { - setUpFakeCluster(2); // HEAD is at v4 - waitForState("version:\\d+ distributor:10 storage:10"); - } - - @Test - void implicit_activation_for_nodes_that_return_not_found_for_version_activation_rpc() throws Exception { - setUpFakeCluster(3); // HEAD is at v4 - waitForState("version:\\d+ distributor:10 storage:10"); - } - - // TODO partial version setup for simulating upgrades - -} diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java index 7db8f47ce5b..5256c952d15 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java @@ -958,7 +958,7 @@ public class StateChangeTest extends FleetControllerTest { builder.setMinTimeBeforeFirstSystemStateBroadcast(3 * 60 * 1000); setUpSystem(builder); boolean useFakeTimer = true; - setUpVdsNodes(useFakeTimer, new DummyVdsNodeOptions(), true); + setUpVdsNodes(useFakeTimer, true); // Leave one node down to avoid sending cluster state due to having seen all node states. for (int i = 0; i < nodes.size(); ++i) { if (i != 3) { @@ -1006,7 +1006,7 @@ public class StateChangeTest extends FleetControllerTest { boolean useFakeTimer = true; setUpSystem(builder); - setUpVdsNodes(useFakeTimer, new DummyVdsNodeOptions(), true); + setUpVdsNodes(useFakeTimer, true); for (DummyVdsNode node : nodes) { node.connect(); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateGatherTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateGatherTest.java index 58df1c1f5eb..796204989b9 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateGatherTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateGatherTest.java @@ -37,9 +37,8 @@ public class StateGatherTest extends FleetControllerTest { .setNodeStateRequestTimeoutLatestPercentage(80); setUpFleetController(true, builder); String[] connectionSpecs = getSlobrokConnectionSpecs(slobrok); - DummyVdsNodeOptions dummyOptions = new DummyVdsNodeOptions(); - DummyVdsNode dnode = new DummyVdsNode(timer, dummyOptions, connectionSpecs, builder.clusterName(), NodeType.DISTRIBUTOR, 0); - DummyVdsNode snode = new DummyVdsNode(timer, dummyOptions, connectionSpecs, builder.clusterName(), NodeType.STORAGE, 0); + DummyVdsNode dnode = new DummyVdsNode(timer, connectionSpecs, builder.clusterName(), NodeType.DISTRIBUTOR, 0); + DummyVdsNode snode = new DummyVdsNode(timer, connectionSpecs, builder.clusterName(), NodeType.STORAGE, 0); dnode.connect(); snode.connect(); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java index 7e0b7f6d953..b533168e61a 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java @@ -1,7 +1,6 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.core.rpc; -import com.yahoo.jrt.ErrorCode; import com.yahoo.jrt.Request; import com.yahoo.jrt.RequestWaiter; import com.yahoo.jrt.Supervisor; @@ -159,32 +158,6 @@ public class RPCCommunicatorTest { } @Test - void set_distribution_states_v3_rpc_auto_downgrades_to_v2_on_unknown_method_error() { - var f = new Fixture<SetClusterStateRequest>(); - var cf = ClusterFixture.forFlatCluster(3).bringEntireClusterUp().assignDummyRpcAddresses(); - var sentBundle = ClusterStateBundleUtil.makeBundle("version:123 distributor:3 storage:3"); - f.communicator.setSystemState(sentBundle, cf.cluster().getNodeInfo(Node.ofStorage(1)), f.mockWaiter); - - RequestWaiter waiter = f.receivedWaiter.get(); - assertNotNull(waiter); - Request req = f.receivedRequest.get(); - assertNotNull(req); - - req.setError(ErrorCode.NO_SUCH_METHOD, "que?"); - waiter.handleRequestDone(req); - - // This would normally be done in processResponses(), but that code path is not invoked in this test. - cf.cluster().getNodeInfo(Node.ofStorage(1)).setClusterStateBundleVersionAcknowledged(123, false); - - f.receivedRequest.set(null); - // Now when we try again, we should have been downgraded to the legacy setsystemstate2 RPC - f.communicator.setSystemState(sentBundle, cf.cluster().getNodeInfo(Node.ofStorage(1)), f.mockWaiter); - req = f.receivedRequest.get(); - assertNotNull(req); - assertEquals(req.methodName(), RPCCommunicator.LEGACY_SET_SYSTEM_STATE2_RPC_METHOD_NAME); - } - - @Test void activateClusterStateVersion_sends_version_activation_rpc() { var f = new Fixture<ActivateClusterStateVersionRequest>(); var cf = ClusterFixture.forFlatCluster(3).bringEntireClusterUp().assignDummyRpcAddresses(); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java index deccaf282d6..211d475de31 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java @@ -128,7 +128,7 @@ public interface WaitCondition { if (!match) { return "Node " + node + " state mismatch.\n wanted: " + pattern + "\n is: " + node.getClusterStateBundle().toString(); } - if (node.getStateCommunicationVersion() > 0 && !node.hasPendingGetNodeStateRequest()) { + if (!node.hasPendingGetNodeStateRequest()) { return "Node " + node + " has not received another get node state request yet"; } } diff --git a/config-lib/src/main/java/com/yahoo/config/ModelReference.java b/config-lib/src/main/java/com/yahoo/config/ModelReference.java index f3adfbfb531..13bb5737c6f 100644 --- a/config-lib/src/main/java/com/yahoo/config/ModelReference.java +++ b/config-lib/src/main/java/com/yahoo/config/ModelReference.java @@ -92,7 +92,7 @@ public class ModelReference { /** * Creates a model reference from a three-part string on the form * <code>modelId url path</code> - * Each of the elements are either a value not containing space, or empty represented by "". + * Each of the elements is either a value not containing space, or empty represented by "". */ public static ModelReference valueOf(String s) { String[] parts = s.split(" "); 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 7973ed93db2..268ca07cdb5 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 @@ -131,6 +131,7 @@ public interface ModelContext { @ModelFeatureFlag(owners = {"tokle"}) default boolean enableProxyProtocolMixedMode() { return true; } @ModelFeatureFlag(owners = {"arnej"}) default String logFileCompressionAlgorithm(String defVal) { return defVal; } @ModelFeatureFlag(owners = {"vekterli"}) default boolean useTwoPhaseDocumentGc() { return false; } + @ModelFeatureFlag(owners = {"hmusum"}) default int clusterControllerStateGatherCount() { return 2; } } /** Warning: As elsewhere in this package, do not make backwards incompatible changes that will break old config models! */ 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 57f01fd8f55..ad816b4b109 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 @@ -154,7 +154,6 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea @Override public boolean useTwoPhaseDocumentGc() { return useTwoPhaseDocumentGc; } @Override public String phraseOptimization() { return phraseOptimization; } - public TestProperties sharedStringRepoNoReclaim(boolean sharedStringRepoNoReclaim) { this.sharedStringRepoNoReclaim = sharedStringRepoNoReclaim; return this; diff --git a/config-model/src/main/java/com/yahoo/schema/processing/SummaryDiskAccessValidator.java b/config-model/src/main/java/com/yahoo/schema/processing/SummaryDiskAccessValidator.java index 40c38a350b0..2294e8f3e09 100644 --- a/config-model/src/main/java/com/yahoo/schema/processing/SummaryDiskAccessValidator.java +++ b/config-model/src/main/java/com/yahoo/schema/processing/SummaryDiskAccessValidator.java @@ -11,7 +11,9 @@ import com.yahoo.vespa.documentmodel.SummaryField; import com.yahoo.vespa.documentmodel.SummaryTransform; import com.yahoo.vespa.model.container.search.QueryProfiles; +import java.util.LinkedHashSet; import java.util.Optional; +import java.util.Set; import java.util.logging.Level; import static com.yahoo.schema.document.ComplexAttributeFieldUtils.isComplexFieldWithOnlyStructFieldAttributes; @@ -38,6 +40,7 @@ public class SummaryDiskAccessValidator extends Processor { for (DocumentSummary summary : schema.getSummaries().values()) { for (SummaryField summaryField : summary.getSummaryFields().values()) { + Set<String> implicitDiskFields = new LinkedHashSet<>(); for (SummaryField.Source source : summaryField.getSources()) { ImmutableSDField field = schema.getField(source.getName()); if (field == null) @@ -45,13 +48,16 @@ public class SummaryDiskAccessValidator extends Processor { if (field == null && ! source.getName().equals(SummaryClass.DOCUMENT_ID_FIELD)) throw new IllegalArgumentException(summaryField + " in " + summary + " references " + source + ", but this field does not exist"); - if ( ! isInMemory(field, summaryField) && ! summary.isFromDisk()) { - deployLogger.logApplicationPackage(Level.WARNING, summaryField + " in " + summary + " references " + - source + ", which is not an attribute: Using this " + - "summary will cause disk accesses. " + - "Set 'from-disk' on this summary class to silence this warning."); - } + if ( ! isInMemory(field, summaryField) && ! summary.isFromDisk()) + implicitDiskFields.add(summaryField.getName()); } + if ( ! implicitDiskFields.isEmpty()) + deployLogger.logApplicationPackage(Level.WARNING, "In " + schema + ", " + summary + + ": Fields " + implicitDiskFields + " references " + + "non-attribute fields: Using this " + + "summary will cause disk accesses. " + + "Set 'from-disk' on this summary class to silence this warning."); + } } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterControllerConfig.java b/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterControllerConfig.java index f0b32f9140d..642b182f0b7 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterControllerConfig.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterControllerConfig.java @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.model.content; +import com.yahoo.config.model.api.ModelContext; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.vespa.config.content.FleetcontrollerConfig; import com.yahoo.config.model.producer.AbstractConfigProducer; @@ -22,11 +23,13 @@ public class ClusterControllerConfig extends AbstractConfigProducer<ClusterContr private final String clusterName; private final ModelElement clusterElement; private final ResourceLimits resourceLimits; + private final int clusterControllerStateGatherCount; - public Builder(String clusterName, ModelElement clusterElement, ResourceLimits resourceLimits) { + public Builder(String clusterName, ModelElement clusterElement, ResourceLimits resourceLimits, ModelContext.FeatureFlags featureFlags) { this.clusterName = clusterName; this.clusterElement = clusterElement; this.resourceLimits = resourceLimits; + this.clusterControllerStateGatherCount = featureFlags.clusterControllerStateGatherCount(); } @Override @@ -52,13 +55,15 @@ public class ClusterControllerConfig extends AbstractConfigProducer<ClusterContr tuning.childAsDouble("min-storage-up-ratio"), bucketSplittingMinimumBits, minNodeRatioPerGroup, - resourceLimits); + resourceLimits, + clusterControllerStateGatherCount); } else { return new ClusterControllerConfig(ancestor, clusterName, null, null, null, null, null, null, bucketSplittingMinimumBits, minNodeRatioPerGroup, - resourceLimits); + resourceLimits, + clusterControllerStateGatherCount); } } } @@ -73,9 +78,10 @@ public class ClusterControllerConfig extends AbstractConfigProducer<ClusterContr private final Integer minSplitBits; private final Double minNodeRatioPerGroup; private final ResourceLimits resourceLimits; + private final int clusterControllerStateGatherCount; // TODO refactor; too many args - private ClusterControllerConfig(AbstractConfigProducer parent, + private ClusterControllerConfig(AbstractConfigProducer<?> parent, String clusterName, Duration initProgressTime, Duration transitionTime, @@ -85,7 +91,8 @@ public class ClusterControllerConfig extends AbstractConfigProducer<ClusterContr Double minStorageUpRatio, Integer minSplitBits, Double minNodeRatioPerGroup, - ResourceLimits resourceLimits) { + ResourceLimits resourceLimits, + int clusterControllerStateGatherCount) { super(parent, "fleetcontroller"); this.clusterName = clusterName; @@ -98,6 +105,7 @@ public class ClusterControllerConfig extends AbstractConfigProducer<ClusterContr this.minSplitBits = minSplitBits; this.minNodeRatioPerGroup = minNodeRatioPerGroup; this.resourceLimits = resourceLimits; + this.clusterControllerStateGatherCount = clusterControllerStateGatherCount; } @Override @@ -140,6 +148,7 @@ public class ClusterControllerConfig extends AbstractConfigProducer<ClusterContr builder.min_node_ratio_per_group(minNodeRatioPerGroup); } resourceLimits.getConfig(builder); + builder.state_gather_count(clusterControllerStateGatherCount); } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java index bf5770681ef..e777999cfbe 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java @@ -127,7 +127,8 @@ public class ContentCluster extends AbstractConfigProducer<AbstractConfigProduce .build(contentElement); c.clusterControllerConfig = new ClusterControllerConfig.Builder(clusterId, contentElement, - resourceLimits.getClusterControllerLimits()) + resourceLimits.getClusterControllerLimits(), + deployState.featureFlags()) .build(deployState, c, contentElement.getXml()); c.search = new ContentSearchCluster.Builder(documentDefinitions, globallyDistributedDocuments, diff --git a/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java b/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java index f33dc46f29b..b5007bba494 100644 --- a/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java +++ b/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java @@ -63,9 +63,10 @@ public class SummaryTestCase { ApplicationBuilder.createFromString(sd, logger); assertEquals(1, logger.entries.size()); assertEquals(Level.WARNING, logger.entries.get(0).level); - assertEquals("summary field 'foo2' in document summary 'foobar' references source field 'ondisk', " + - "which is not an attribute: Using this summary will cause disk accesses. " + - "Set 'from-disk' on this summary class to silence this warning.", + assertEquals("In schema 'disksummary', document summary 'foobar': " + + "Fields [foo2] references non-attribute fields: " + + "Using this summary will cause disk accesses. " + + "Set 'from-disk' on this summary class to silence this warning.", logger.entries.get(0).message); } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigValueChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigValueChangeValidatorTest.java index 83672b6452e..a08023f2e10 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigValueChangeValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigValueChangeValidatorTest.java @@ -24,7 +24,6 @@ import org.junit.jupiter.api.Test; import java.time.Instant; import java.util.Arrays; import java.util.List; -import java.util.Map; import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.*; diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java index 1e6847a47be..b449d5403b5 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java @@ -27,7 +27,8 @@ public class FleetControllerClusterTest { new ClusterResourceLimits.Builder(false, featureFlags.resourceLimitDisk(), featureFlags.resourceLimitMemory()) - .build(clusterElement).getClusterControllerLimits()) + .build(clusterElement).getClusterControllerLimits(), + new TestProperties().featureFlags()) .build(root.getDeployState(), root, clusterElement.getXml()); } diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java b/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java index 2e62a0decbe..80e33fee874 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java @@ -45,7 +45,7 @@ public class Cloud { /** For testing purposes only */ public static Cloud defaultCloud() { - return new Builder().name(CloudName.defaultName()).build(); + return new Builder().name(CloudName.DEFAULT).build(); } public static Builder builder() { @@ -54,7 +54,7 @@ public class Cloud { public static class Builder { - private CloudName name = CloudName.defaultName(); + private CloudName name = CloudName.DEFAULT; private boolean dynamicProvisioning = false; private boolean reprovisionToUpgradeOs = false; private boolean requireAccessControl = false; diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/CloudName.java b/config-provisioning/src/main/java/com/yahoo/config/provision/CloudName.java index 417e381587e..f7edce03525 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/CloudName.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/CloudName.java @@ -14,20 +14,14 @@ import java.util.regex.Pattern; public class CloudName extends PatternedStringWrapper<CloudName> { private static final Pattern pattern = Pattern.compile("[a-z]([a-z0-9-]*[a-z0-9])*"); - private static final CloudName defaultCloud = from("default"); + public static final CloudName AWS = from("aws"); + public static final CloudName GCP = from("gcp"); + public static final CloudName DEFAULT = from("default"); private CloudName(String cloud) { super(cloud, pattern, "cloud name"); } - public boolean isDefault() { - return equals(defaultCloud); - } - - public static CloudName defaultName() { - return defaultCloud; - } - public static CloudName from(String cloud) { return new CloudName(cloud); } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java index 59a48ad3c7e..df449ca017b 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java @@ -277,6 +277,7 @@ public class Deployment implements com.yahoo.config.provision.Deployment { .tenantSecretStores(session.getTenantSecretStores()); session.getDockerImageRepository().ifPresent(params::dockerImageRepository); session.getAthenzDomain().ifPresent(params::athenzDomain); + session.getCloudAccount().ifPresent(params::cloudAccount); return params.build(); }); 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 48450716131..683244e5e2f 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 @@ -217,12 +217,13 @@ public class ModelContextImpl implements ModelContext { private final boolean mbus_dispatch_on_encode; private final int mbus_threads; private final int mbus_network_threads; - private int mbus_java_num_targets; - private int mbus_java_events_before_wakeup; - private int mbus_cpp_num_targets; - private int mbus_cpp_events_before_wakeup; - private int rpc_num_targets; - private int rpc_events_before_wakeup; + private final int mbus_java_num_targets; + private final int mbus_java_events_before_wakeup; + private final int mbus_cpp_num_targets; + private final int mbus_cpp_events_before_wakeup; + private final int rpc_num_targets; + private final int rpc_events_before_wakeup; + private final int clusterControllerStateGatherCount; public FeatureFlags(FlagSource source, ApplicationId appId, Version version) { this.defaultTermwiseLimit = flagValue(source, appId, version, Flags.DEFAULT_TERM_WISE_LIMIT); @@ -282,6 +283,7 @@ public class ModelContextImpl implements ModelContext { this.rpc_events_before_wakeup = flagValue(source, appId, version, Flags.RPC_EVENTS_BEFORE_WAKEUP); this.queryDispatchPolicy = flagValue(source, appId, version, Flags.QUERY_DISPATCH_POLICY); this.phraseOptimization = flagValue(source, appId, version, Flags.PHRASE_OPTIMIZATION); + this.clusterControllerStateGatherCount = flagValue(source, appId, version, Flags.CLUSTER_CONTROLLER_STATE_GATHER_COUNT); } @Override public String queryDispatchPolicy() { return queryDispatchPolicy; } @@ -349,6 +351,7 @@ public class ModelContextImpl implements ModelContext { return defVal; } @Override public boolean useTwoPhaseDocumentGc() { return useTwoPhaseDocumentGc; } + @Override public int clusterControllerStateGatherCount() { return clusterControllerStateGatherCount; } private static <V> V flagValue(FlagSource source, ApplicationId appId, Version vespaVersion, UnboundFlag<? extends V, ?, ?> flag) { return flag.bindTo(source) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java index e146f0de187..988d13b1978 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java @@ -325,6 +325,8 @@ public class SessionZooKeeperClient { if (cloudAccount.isPresent()) { byte[] data = uncheck(() -> SlimeUtils.toJsonBytes(CloudAccountSerializer.toSlime(cloudAccount.get()))); curator.set(cloudAccountPath(), data); + } else { + curator.delete(cloudAccountPath()); } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/noderepository/NodeRepositoryNode.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/noderepository/NodeRepositoryNode.java index d8a804ecd2c..1a1d283d6eb 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/noderepository/NodeRepositoryNode.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/noderepository/NodeRepositoryNode.java @@ -107,6 +107,8 @@ public class NodeRepositoryNode { private String exclusiveTo; @JsonProperty("switchHostname") private String switchHostname; + @JsonProperty("cloudAccount") + private String cloudAccount; public String getUrl() { return url; @@ -438,6 +440,13 @@ public class NodeRepositoryNode { this.switchHostname = switchHostname; } + public String getCloudAccount() { + return cloudAccount; + } + + public void setCloudAccount(String cloudAccount) { + this.cloudAccount = cloudAccount; + } // --- Helper methods for code that (wrongly) consume this directly @@ -497,6 +506,7 @@ public class NodeRepositoryNode { ", reservedTo='" + reservedTo + '\'' + ", exclusiveTo='" + exclusiveTo + '\'' + ", switchHostname='" + switchHostname + '\'' + + ", cloudAccount='" + cloudAccount + '\'' + '}'; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java index bdb68f655ff..e3a0ad7cb84 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java @@ -173,7 +173,7 @@ public class Application { /** * Returns the oldest platform version this has deployed in a permanent zone (not test or staging). * - * This is unfortunately quite similar to {@link ApplicationController#oldestInstalledPlatform(TenantAndApplicationId)}, + * This is unfortunately quite similar to {@link ApplicationController#oldestInstalledPlatform(Application)}, * but this checks only what the controller has deployed to the production zones, while that checks the node repository * to see what's actually installed on each node. Thus, this is the right choice for, e.g., target Vespa versions for * new deployments, while that is the right choice for version to compile against. diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index 78063a383dc..047c059e9a6 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -3,7 +3,9 @@ package com.yahoo.vespa.hosted.controller; import com.yahoo.component.Version; import com.yahoo.component.VersionCompatibility; +import com.yahoo.config.application.api.DeploymentInstanceSpec; import com.yahoo.config.application.api.DeploymentSpec; +import com.yahoo.config.application.api.DeploymentSpec.UpgradePolicy; import com.yahoo.config.application.api.ValidationId; import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; @@ -57,6 +59,7 @@ import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackageValid import com.yahoo.vespa.hosted.controller.athenz.impl.AthenzFacade; import com.yahoo.vespa.hosted.controller.certificate.EndpointCertificates; import com.yahoo.vespa.hosted.controller.concurrent.Once; +import com.yahoo.vespa.hosted.controller.deployment.DeploymentStatus; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger; import com.yahoo.vespa.hosted.controller.deployment.JobStatus; import com.yahoo.vespa.hosted.controller.deployment.Run; @@ -71,6 +74,7 @@ import com.yahoo.vespa.hosted.controller.tenant.CloudTenant; import com.yahoo.vespa.hosted.controller.tenant.Tenant; import com.yahoo.vespa.hosted.controller.versions.VersionStatus; import com.yahoo.vespa.hosted.controller.versions.VespaVersion; +import com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence; import com.yahoo.yolean.Exceptions; import java.security.Principal; @@ -99,7 +103,10 @@ import java.util.stream.Collectors; import static com.yahoo.vespa.flags.FetchVector.Dimension.APPLICATION_ID; import static com.yahoo.vespa.hosted.controller.api.integration.configserver.Node.State.active; import static com.yahoo.vespa.hosted.controller.api.integration.configserver.Node.State.reserved; +import static com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence.broken; +import static com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence.high; import static com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence.low; +import static com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence.normal; import static java.util.Comparator.naturalOrder; import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; @@ -231,16 +238,6 @@ public class ApplicationController { return curator.readApplications(false); } - /** Returns the target major version for applications not specifying one */ - public OptionalInt targetMajorVersion() { - return curator.readTargetMajorVersion().map(OptionalInt::of).orElse(OptionalInt.empty()); - } - - /** Sets the default target major version. Set to empty to determine target version normally (by confidence) */ - public void setTargetMajorVersion(OptionalInt targetMajorVersion) { - curator.writeTargetMajorVersion(targetMajorVersion); - } - /** * Returns a snapshot of all readable applications. Unlike {@link ApplicationController#asList()} this ignores * applications that cannot currently be read (e.g. due to serialization issues) and may return an incomplete @@ -304,67 +301,80 @@ public class ApplicationController { } /** Returns the oldest Vespa version installed on any active or reserved production node for the given application. */ - public Version oldestInstalledPlatform(TenantAndApplicationId id) { - return controller.jobController().deploymentStatus(requireApplication(id)).jobs() - .production().asList().stream() + public Optional<Version> oldestInstalledPlatform(Application application) { + return controller.jobController().deploymentStatus(application).jobs() + .production().asList() + .stream() .map(this::oldestInstalledPlatform) .flatMap(Optional::stream) - .min(naturalOrder()) - .orElse(controller.readSystemVersion()); + .min(naturalOrder()); } /** - * Returns the preferred Vespa version to compile against, for the given application, on an optionally restricted major. - * - * A target major may be specified as an argument; or it may be set specifically for the application; - * or in general, for all applications; the first such specification wins. - * + * Returns the preferred Vespa version to compile against, for the given application, optionally restricted to a major. + * <p> * The returned version is not newer than the oldest deployed platform for the application, unless * the target major differs from the oldest deployed platform, in which case it is not newer than * the oldest available platform version on that major instead. - * + * <p> * The returned version is compatible with a platform version available in the system. - * + * <p> * A candidate is sought first among versions with non-broken confidence, then among those with forgotten confidence. - * + * <p> * The returned version is the latest in the relevant candidate set. - * + * <p> * If no such version exists, an {@link IllegalArgumentException} is thrown. */ public Version compileVersion(TenantAndApplicationId id, OptionalInt wantedMajor) { // Read version status, and pick out target platforms we could run the compiled package on. - OptionalInt targetMajor = firstNonEmpty(wantedMajor, requireApplication(id).majorVersion(), targetMajorVersion()); + Optional<Application> application = getApplication(id); + Optional<Version> oldestInstalledPlatform = application.flatMap(this::oldestInstalledPlatform); VersionStatus versionStatus = controller.readVersionStatus(); - Version systemVersion = controller.systemVersion(versionStatus); - Version oldestInstalledPlatform = oldestInstalledPlatform(id); + UpgradePolicy policy = application.flatMap(app -> app.deploymentSpec().instances().stream() + .map(DeploymentInstanceSpec::upgradePolicy) + .max(naturalOrder())) + .orElse(UpgradePolicy.defaultPolicy); + Confidence targetConfidence = switch (policy) { + case canary -> broken; + case defaultPolicy -> normal; + case conservative -> high; + }; // Target platforms are all versions not older than the oldest installed platform, unless forcing a major version change. - // Only major version specified in deployment spec is enough to force a downgrade, while all sources may force an upgrade. - Predicate<Version> isTargetPlatform = targetMajor.isEmpty() - || targetMajor.getAsInt() == oldestInstalledPlatform.getMajor() - || wantedMajor.isEmpty() && targetMajor.getAsInt() <= oldestInstalledPlatform.getMajor() - ? version -> ! version.isBefore(oldestInstalledPlatform) - : version -> targetMajor.getAsInt() == version.getMajor(); - Set<Version> platformVersions = versionStatus.versions().stream() + // Only platforms not older than the system version, and with appropriate confidence, are considered targets. + Predicate<Version> isTargetPlatform = wantedMajor.isEmpty() && oldestInstalledPlatform.isEmpty() + ? __ -> true + : wantedMajor.isEmpty() || wantedMajor.getAsInt() == oldestInstalledPlatform.get().getMajor() + ? version -> ! version.isBefore(oldestInstalledPlatform.get()) + : version -> wantedMajor.getAsInt() == version.getMajor(); + Set<Version> platformVersions = versionStatus.deployableVersions().stream() + .filter(version -> version.confidence().equalOrHigherThan(targetConfidence)) .map(VespaVersion::versionNumber) - .filter(version -> ! version.isAfter(systemVersion)) .filter(isTargetPlatform) .collect(toSet()); + oldestInstalledPlatform.ifPresent(fallback -> { + if (wantedMajor.isEmpty() || wantedMajor.getAsInt() == fallback.getMajor()) + platformVersions.add(fallback); + }); + if (platformVersions.isEmpty()) throw new IllegalArgumentException("this system has no available versions" + - (targetMajor.isPresent() ? " on specified major: " + targetMajor.getAsInt() : "")); + (wantedMajor.isPresent() ? " on specified major: " + wantedMajor.getAsInt() : "")); // The returned compile version must be compatible with at least one target platform. // If it is incompatible with any of the current platforms, the system will trigger a platform change. // The returned compile version should also be at least as old as both the oldest target platform version, - // and the oldest current platform, unless the two are incompatible, in which case only the target matters. + // and the oldest current platform, unless the two are incompatible, in which case only the target matters, + // or there are no installed platforms, in which case we prefer the newest target platform. VersionCompatibility compatibility = versionCompatibility(id.defaultInstance()); // Wrong id level >_< Version oldestTargetPlatform = platformVersions.stream().min(naturalOrder()).get(); - Version newestVersion = compatibility.accept(oldestInstalledPlatform, oldestTargetPlatform) - && oldestInstalledPlatform.isBefore(oldestTargetPlatform) - ? oldestInstalledPlatform - : oldestTargetPlatform; + Version newestVersion = oldestInstalledPlatform.isEmpty() + ? platformVersions.stream().max(naturalOrder()).get() + : compatibility.accept(oldestInstalledPlatform.get(), oldestTargetPlatform) + && oldestInstalledPlatform.get().isBefore(oldestTargetPlatform) + ? oldestInstalledPlatform.get() + : oldestTargetPlatform; Predicate<Version> systemCompatible = version -> ! version.isAfter(newestVersion) && platformVersions.stream().anyMatch(platform -> compatibility.accept(platform, version)); @@ -386,7 +396,7 @@ public class ApplicationController { if (unknown.isPresent()) return unknown.get(); throw new IllegalArgumentException("no suitable, released compile version exists" + - (targetMajor.isPresent() ? " for specified major: " + targetMajor.getAsInt() : "")); + (wantedMajor.isPresent() ? " for specified major: " + wantedMajor.getAsInt() : "")); } private OptionalInt firstNonEmpty(OptionalInt... choices) { @@ -966,15 +976,6 @@ public class ApplicationController { throw new IllegalArgumentException("Not allowed to launch Athenz service " + athenzService.getFullName()); } - /** Returns the latest known version within the given major, which is not newer than the system version. */ - public Optional<Version> lastCompatibleVersion(int targetMajorVersion) { - VersionStatus versions = controller.readVersionStatus(); - return versions.deployableVersions().stream() - .map(VespaVersion::versionNumber) - .filter(version -> version.getMajor() == targetMajorVersion) - .max(naturalOrder()); - } - /** Extract deployment warnings metric from deployment result */ private static Map<DeploymentMetrics.Warning, Integer> warningsFrom(ActivateResult result) { if (result.prepareResponse().log == null) return Map.of(); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java index 153ce87ff6b..626937dc6ac 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java @@ -52,15 +52,18 @@ public class InstanceList extends AbstractFilteringList<ApplicationId, InstanceL } /** - * Returns the subset of instances that aren't pinned to an earlier major version than the given one. + * Returns the subset of instances whose application have a deployment on the given major, or specify it in deployment spec. * * @param targetMajorVersion the target major version which applications returned allows upgrading to - * @param defaultMajorVersion the default major version to assume for applications not specifying one */ - public InstanceList allowingMajorVersion(int targetMajorVersion, int defaultMajorVersion) { - return matching(id -> targetMajorVersion <= application(id).deploymentSpec().majorVersion() - .orElse(application(id).majorVersion() - .orElse(defaultMajorVersion))); + public InstanceList allowingMajorVersion(int targetMajorVersion) { + return matching(id -> { + Application application = application(id); + return application.deploymentSpec().majorVersion().map(allowed -> targetMajorVersion <= allowed) + .orElseGet(() -> application.productionDeployments().values().stream() + .flatMap(List::stream) + .anyMatch(deployment -> targetMajorVersion <= deployment.version().getMajor())); + }); } /** Returns the subset of instances that are allowed to upgrade to the given version at the given time */ diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java index b3df417bd80..5a131ba4a29 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java @@ -116,13 +116,18 @@ public class ApplicationPackageValidator { var clouds = new HashSet<CloudName>(); for (var region : endpoint.regions()) { for (ZoneApi zone : controller.zoneRegistry().zones().all().in(Environment.prod).in(region).zones()) { + if (zone.getCloudName().equals(CloudName.GCP)) { + throw new IllegalArgumentException("Endpoint '" + endpoint.endpointId() + "' in " + instance + + " contains a Google Cloud region (" + region + + "), which is not yet supported"); + } clouds.add(zone.getCloudName()); } } if (clouds.size() != 1) { throw new IllegalArgumentException("Endpoint '" + endpoint.endpointId() + "' in " + instance + " cannot contain regions in different clouds: " + - endpoint.regions().stream().sorted().collect(Collectors.toList())); + endpoint.regions().stream().sorted().toList()); } } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java index 0df70cd9c53..4c869a5aabe 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java @@ -63,7 +63,7 @@ public class EndpointCertificates { if (duration.toSeconds() > 30) log.log(Level.INFO, Text.format("Getting endpoint certificate metadata for %s took %d seconds!", instance.id().serializedForm(), duration.toSeconds())); - if (controller.zoneRegistry().zones().all().in(CloudName.from("gcp")).ids().contains(zone)) { // Until CKMS is available from GCP + if (controller.zoneRegistry().zones().all().in(CloudName.GCP).ids().contains(zone)) { // Until CKMS is available from GCP if(metadata.isPresent()) { // Validate metadata before copying cert to GCP. This will ensure we don't bug out on the first deployment, but will take more time certificateValidator.validate(metadata.get(), instance.id().serializedForm(), zone, controller.routing().certificateDnsNames(new DeploymentId(instance.id(), zone), deploymentSpec)); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java index e5e5c105614..50492077e20 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java @@ -185,7 +185,7 @@ public class DeploymentStatus { || application.productionDeployments().getOrDefault(instance, List.of()).stream() .anyMatch(deployment -> ! compatibleWithCompileVersion.test(deployment.version()))) { return targetsForPolicy(versionStatus, systemVersion, application.deploymentSpec().requireInstance(instance).upgradePolicy()) - .stream() // Pick the latest platform which is compatible with the compile version, and is ready for this instance. + .stream() // Pick the latest platform with appropriate confidence, which is compatible with the compile version. .filter(compatibleWithCompileVersion) .findFirst() .map(platform -> change.withoutPin().with(platform)) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceTagMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceTagMaintainer.java index 3588ae53a74..705abd9ed56 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceTagMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceTagMaintainer.java @@ -35,7 +35,7 @@ public class ResourceTagMaintainer extends ControllerMaintainer { public double maintain() { controller().zoneRegistry().zones() .reachable() - .in(CloudName.from("aws")) + .in(CloudName.AWS) .zones().forEach(zone -> { Map<HostName, ApplicationId> applicationOfHosts = getTenantOfParentHosts(zone.getId()); int taggedResources = resourceTagger.tagResources(zone, applicationOfHosts); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java index ce98643c25a..f3c82e72ae3 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java @@ -61,17 +61,16 @@ public class Upgrader extends ControllerMaintainer { VersionStatus versionStatus = controller().readVersionStatus(); cancelBrokenUpgrades(versionStatus); - OptionalInt targetMajorVersion = targetMajorVersion(); DeploymentStatusList deploymentStatuses = deploymentStatuses(versionStatus); for (UpgradePolicy policy : UpgradePolicy.values()) - updateTargets(versionStatus, deploymentStatuses, policy, targetMajorVersion); + updateTargets(versionStatus, deploymentStatuses, policy); return 1.0; } private DeploymentStatusList deploymentStatuses(VersionStatus versionStatus) { return controller().jobController().deploymentStatuses(ApplicationList.from(controller().applications().readable()) - .withProjectId(), + .withProjectId(), versionStatus); } @@ -94,7 +93,7 @@ public class Upgrader extends ControllerMaintainer { } } - private void updateTargets(VersionStatus versionStatus, DeploymentStatusList deploymentStatuses, UpgradePolicy policy, OptionalInt targetMajorVersion) { + private void updateTargets(VersionStatus versionStatus, DeploymentStatusList deploymentStatuses, UpgradePolicy policy) { InstanceList instances = instances(deploymentStatuses); InstanceList remaining = instances.with(policy); Instant failureThreshold = controller().clock().instant().minus(DeploymentTrigger.maxFailingRevisionTime); @@ -108,7 +107,7 @@ public class Upgrader extends ControllerMaintainer { Map<ApplicationId, Version> targets = new LinkedHashMap<>(); for (Version version : DeploymentStatus.targetsForPolicy(versionStatus, controller().systemVersion(versionStatus), policy)) { targetAndNewer.add(version); - InstanceList eligible = eligibleForVersion(remaining, version, targetMajorVersion); + InstanceList eligible = eligibleForVersion(remaining, version); InstanceList outdated = cancellationCriterion.apply(eligible); cancelUpgradesOf(outdated.upgrading(), "Upgrading to outdated versions"); @@ -135,11 +134,10 @@ public class Upgrader extends ControllerMaintainer { } } - private InstanceList eligibleForVersion(InstanceList instances, Version version, - OptionalInt targetMajorVersion) { + private InstanceList eligibleForVersion(InstanceList instances, Version version) { Change change = Change.of(version); return instances.not().failingOn(version) - .allowingMajorVersion(version.getMajor(), targetMajorVersion.orElse(version.getMajor())) + .allowingMajorVersion(version.getMajor()) .compatibleWithPlatform(version, controller().applications()::versionCompatibility) .not().hasCompleted(change) // Avoid rescheduling change for instances without production steps. .onLowerVersionThan(version) @@ -182,16 +180,6 @@ public class Upgrader extends ControllerMaintainer { curator.writeUpgradesPerMinute(n); } - /** Returns the target major version for applications not specifying one */ - public OptionalInt targetMajorVersion() { - return controller().applications().targetMajorVersion(); - } - - /** Sets the default target major version. Set to empty to determine target version normally (by confidence) */ - public void setTargetMajorVersion(OptionalInt targetMajorVersion) { - controller().applications().setTargetMajorVersion(targetMajorVersion); - } - /** Override confidence for given version. This will cause the computed confidence to be ignored */ public void overrideConfidence(Version version, Confidence confidence) { if (confidence == Confidence.aborted && !version.isAfter(controller().readSystemVersion())) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java index 54e98877ba3..38cea6f8cef 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java @@ -269,17 +269,6 @@ public class CuratorDb { curator.set(upgradesPerMinutePath(), ByteBuffer.allocate(Double.BYTES).putDouble(n).array()); } - public Optional<Integer> readTargetMajorVersion() { - return read(targetMajorVersionPath(), ByteBuffer::wrap).map(ByteBuffer::getInt); - } - - public void writeTargetMajorVersion(OptionalInt targetMajorVersion) { - if (targetMajorVersion.isPresent()) - curator.set(targetMajorVersionPath(), ByteBuffer.allocate(Integer.BYTES).putInt(targetMajorVersion.getAsInt()).array()); - else - curator.delete(targetMajorVersionPath()); - } - public void writeVersionStatus(VersionStatus status) { curator.set(versionStatusPath(), asJson(versionStatusSerializer.toSlime(status))); } @@ -697,10 +686,6 @@ public class CuratorDb { return root.append("upgrader").append("upgradesPerMinute"); } - private static Path targetMajorVersionPath() { - return root.append("upgrader").append("targetMajorVersion"); - } - private static Path confidenceOverridesPath() { return root.append("upgrader").append("confidenceOverrides"); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java index 9278d030db6..2b4c6411386 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java @@ -156,7 +156,6 @@ public class ControllerApiHandler extends AuditLoggingRequestHandler { private HttpResponse configureUpgrader(HttpRequest request) { String upgradesPerMinuteField = "upgradesPerMinute"; - String targetMajorVersionField = "targetMajorVersion"; byte[] jsonBytes = toJsonBytes(request.getData()); Inspector inspect = SlimeUtils.jsonToSlime(jsonBytes).get(); @@ -164,9 +163,6 @@ public class ControllerApiHandler extends AuditLoggingRequestHandler { if (inspect.field(upgradesPerMinuteField).valid()) { upgrader.setUpgradesPerMinute(inspect.field(upgradesPerMinuteField).asDouble()); - } else if (inspect.field(targetMajorVersionField).valid()) { - int target = (int) inspect.field(targetMajorVersionField).asLong(); - upgrader.setTargetMajorVersion(target == 0 ? OptionalInt.empty() : OptionalInt.of(target)); // 0 is the default value } else { return ErrorResponse.badRequest("No such modifiable field(s)"); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/UpgraderResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/UpgraderResponse.java index 122ea94ab6c..28fa5183e5c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/UpgraderResponse.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/UpgraderResponse.java @@ -19,7 +19,6 @@ public class UpgraderResponse extends SlimeJsonResponse { Slime slime = new Slime(); Cursor root = slime.setObject(); root.setDouble("upgradesPerMinute", upgrader.upgradesPerMinute()); - upgrader.targetMajorVersion().ifPresent(v -> root.setLong("targetMajorVersion", v)); Cursor array = root.setArray("confidenceOverrides"); upgrader.confidenceOverrides().forEach((version, confidence) -> { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VespaVersion.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VespaVersion.java index e078df0267f..4cb10aa6ba9 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VespaVersion.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VespaVersion.java @@ -31,10 +31,9 @@ public record VespaVersion(Version version, public static Confidence confidenceFrom(DeploymentStatistics statistics, Controller controller) { int thisMajorVersion = statistics.version().getMajor(); - int defaultMajorVersion = controller.applications().targetMajorVersion().orElse(thisMajorVersion); InstanceList all = InstanceList.from(controller.jobController().deploymentStatuses(ApplicationList.from(controller.applications().asList()) .withProductionDeployment())) - .allowingMajorVersion(thisMajorVersion, defaultMajorVersion); + .allowingMajorVersion(thisMajorVersion); // 'production on this': All production deployment jobs upgrading to this version have completed without failure InstanceList productionOnThis = all.matching(instance -> statistics.productionSuccesses().stream().anyMatch(run -> run.id().application().equals(instance))) .not().failingUpgrade() diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java index 6c193e9b539..e09e42c9900 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java @@ -49,6 +49,7 @@ import com.yahoo.vespa.hosted.controller.routing.RoutingStatus; import com.yahoo.vespa.hosted.controller.routing.context.DeploymentRoutingContext; import com.yahoo.vespa.hosted.controller.routing.rotation.RotationId; import com.yahoo.vespa.hosted.controller.routing.rotation.RotationLock; +import com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence; import com.yahoo.vespa.hosted.rotation.config.RotationsConfig; import org.junit.jupiter.api.Test; @@ -75,6 +76,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -874,7 +876,7 @@ public class ControllerTest { ZoneApiMock.fromId("test.us-west-1"), ZoneApiMock.fromId("staging.us-west-1"), ZoneApiMock.fromId("prod.us-west-1"), - ZoneApiMock.newBuilder().with(CloudName.from("aws")).withId("prod.aws-us-east-1").build() + ZoneApiMock.newBuilder().with(CloudName.AWS).withId("prod.aws-us-east-1").build() ); var context = tester.newDeploymentContext(); var applicationPackage = new ApplicationPackageBuilder() @@ -905,6 +907,38 @@ public class ControllerTest { } @Test + void testDeployWithGlobalEndpointsInGcp() { + tester.controllerTester().zoneRegistry().setZones( + ZoneApiMock.fromId("test.us-west-1"), + ZoneApiMock.fromId("staging.us-west-1"), + ZoneApiMock.newBuilder().with(CloudName.GCP).withId("prod.gcp-us-east1-b").build() + ); + var context = tester.newDeploymentContext(); + var applicationPackage = new ApplicationPackageBuilder() + .region("gcp-us-east1-b") + .endpoint("default", "default") // Contains all regions by default + .build(); + + try { + context.submit(applicationPackage); + fail("Expected exception"); + } catch (IllegalArgumentException e) { + assertEquals("Endpoint 'default' in instance 'default' contains a Google Cloud region (gcp-us-east1-b), which is not yet supported", e.getMessage()); + } + + var applicationPackage2 = new ApplicationPackageBuilder() + .region("gcp-us-east1-b") + .endpoint("gcp", "default", "gcp-us-east1-b") + .build(); + try { + context.submit(applicationPackage2); + fail("Expected exception"); + } catch (IllegalArgumentException e) { + assertEquals("Endpoint 'gcp' in instance 'default' contains a Google Cloud region (gcp-us-east1-b), which is not yet supported", e.getMessage()); + } + } + + @Test void testDeployWithoutSourceRevision() { var context = tester.newDeploymentContext(); var applicationPackage = new ApplicationPackageBuilder() @@ -1130,12 +1164,16 @@ public class ControllerTest { // No deployments result in system version Version version0 = Version.fromString("7.1"); tester.controllerTester().upgradeSystem(version0); + tester.upgrader().overrideConfidence(version0, Confidence.normal); + tester.controllerTester().computeVersionStatus(); assertEquals(version0, tester.applications().compileVersion(application, OptionalInt.empty())); context.submit(applicationPackage).deploy(); // System is upgraded Version version1 = Version.fromString("7.2"); tester.controllerTester().upgradeSystem(version1); + tester.upgrader().overrideConfidence(version1, Confidence.normal); + tester.controllerTester().computeVersionStatus(); assertEquals(version0, tester.applications().compileVersion(application, OptionalInt.empty())); // Application is upgraded and compile version is bumped @@ -1143,47 +1181,86 @@ public class ControllerTest { context.deployPlatform(version1); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.empty())); + DeploymentContext legacyApp = tester.newDeploymentContext("avoid", "gc", "default").submit().deploy(); + TenantAndApplicationId newApp = TenantAndApplicationId.from("new", "app"); + // A new major is released to the system Version version2 = Version.fromString("8.0"); tester.controllerTester().upgradeSystem(version2); + tester.upgrader().overrideConfidence(version2, Confidence.low); + tester.controllerTester().computeVersionStatus(); + assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.empty())); + assertEquals("this system has no available versions on specified major: 8", + assertThrows(IllegalArgumentException.class, + () -> tester.applications().compileVersion(application, OptionalInt.of(8))) + .getMessage()); + + tester.upgrader().overrideConfidence(version2, Confidence.normal); + tester.controllerTester().computeVersionStatus(); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.empty())); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(8))); + assertEquals(version2, tester.applications().compileVersion(newApp, OptionalInt.empty())); // The new major is marked as incompatible with older compile versions tester.controllerTester().flagSource().withListFlag(PermanentFlags.INCOMPATIBLE_VERSIONS.id(), List.of("8"), String.class); - assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.of(8))); - - // Default major version is set to 8. - tester.applications().setTargetMajorVersion(OptionalInt.of(8)); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); - assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.empty())); - - // Application sets target major to 7. - tester.applications().lockApplicationOrThrow(application, locked -> tester.applications().store(locked.withMajorVersion(7))); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.empty())); assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.of(8))); + assertEquals(version2, tester.applications().compileVersion(newApp, OptionalInt.empty())); - // Application sets target major to 8. - tester.applications().lockApplicationOrThrow(application, locked -> tester.applications().store(locked.withMajorVersion(8))); + // The only version on major 8 has low confidence. + tester.upgrader().overrideConfidence(version2, Confidence.low); + tester.controllerTester().computeVersionStatus(); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); - assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.empty())); - - // Application upgrades to major 8; only major version from deployment spec should cause a downgrade. + assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.empty())); + assertEquals("this system has no available versions on specified major: 8", + assertThrows(IllegalArgumentException.class, + () -> tester.applications().compileVersion(application, OptionalInt.of(8))) + .getMessage()); + assertEquals(version1, tester.applications().compileVersion(newApp, OptionalInt.empty())); + assertEquals(version1, tester.applications().compileVersion(newApp, OptionalInt.empty())); + + // Version on major 8 has normal confidence again + tester.upgrader().overrideConfidence(version2, Confidence.normal); + tester.controllerTester().computeVersionStatus(); + + // Application upgrades to major 8; major version from deployment spec should cause a downgrade. context.submit(new ApplicationPackageBuilder().region("us-west-1").compileVersion(version2).build()).deploy(); - tester.applications().setTargetMajorVersion(OptionalInt.empty()); - tester.applications().lockApplicationOrThrow(application, locked -> tester.applications().store(locked.withMajorVersion(null))); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.empty())); - // Default major version across all apps should not cause a downgrade. - tester.applications().setTargetMajorVersion(OptionalInt.of(7)); + // Reduced confidence should not cause a downgrade. + tester.upgrader().overrideConfidence(version2, Confidence.low); + tester.controllerTester().computeVersionStatus(); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.empty())); + assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.of(8))); + + // All versions on new major having broken confidence makes it all fail for upgraded apps, but this shouldn't happen in practice. + tester.upgrader().overrideConfidence(version2, Confidence.broken); + tester.controllerTester().computeVersionStatus(); + assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); + assertEquals("no suitable, released compile version exists", + assertThrows(IllegalArgumentException.class, + () -> tester.applications().compileVersion(application, OptionalInt.empty())) + .getMessage()); + assertEquals("no suitable, released compile version exists for specified major: 8", + assertThrows(IllegalArgumentException.class, + () -> tester.applications().compileVersion(application, OptionalInt.of(8))) + .getMessage()); + + // Major versions are not incompatible anymore, so the old compile version should work again. + tester.controllerTester().flagSource().withListFlag(PermanentFlags.INCOMPATIBLE_VERSIONS.id(), List.of(), String.class); + assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); + assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.empty())); + assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(8))); - // Default major version for specific app should not cause a downgrade. - tester.applications().lockApplicationOrThrow(application, locked -> tester.applications().store(locked.withMajorVersion(7))); + // Simply reduced confidence shouldn't cause any changes. + tester.upgrader().overrideConfidence(version2, Confidence.low); + tester.controllerTester().computeVersionStatus(); assertEquals(version1, tester.applications().compileVersion(application, OptionalInt.of(7))); assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.empty())); + assertEquals(version2, tester.applications().compileVersion(application, OptionalInt.of(8))); } @Test diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java index fda1f5f0b77..3b8d0374a45 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java @@ -164,7 +164,7 @@ public class DeploymentTriggerTest { app.submit(applicationPackage).deploy(); - Change upgrade = Change.of(new Version("7.8.9")); + Change upgrade = Change.of(new Version("6.8.9")); tester.controllerTester().upgradeSystem(upgrade.platform().get()); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest); @@ -209,7 +209,7 @@ public class DeploymentTriggerTest { app.runJob(productionUsCentral1).runJob(productionUsWest1).runJob(productionUsEast3); assertEquals(Change.empty(), app.instance().change()); - tester.controllerTester().upgradeSystem(new Version("8.9")); + tester.controllerTester().upgradeSystem(new Version("6.9")); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest); tester.clock().advance(Duration.ofMinutes(1)); @@ -545,7 +545,7 @@ public class DeploymentTriggerTest { .region("us-east-3") .build(); var app = tester.newDeploymentContext().submit(applicationPackage).deploy(); - tester.controllerTester().upgradeSystem(new Version("9.8.7")); + tester.controllerTester().upgradeSystem(new Version("6.8.7")); tester.upgrader().maintain(); tester.deploymentTrigger().pauseJob(app.instanceId(), productionUsWest1, @@ -1118,9 +1118,8 @@ public class DeploymentTriggerTest { DeploymentContext alpha = tester.newDeploymentContext("t", "a", "alpha"); DeploymentContext beta = tester.newDeploymentContext("t", "a", "beta"); alpha.submit(applicationPackage).deploy(); - Optional<RevisionId> revision1 = alpha.lastSubmission(); - Version version1 = new Version("7.1"); + Version version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().run(); alpha.runJob(systemTest).runJob(stagingTest); @@ -1204,7 +1203,7 @@ public class DeploymentTriggerTest { // Application starts upgrade, but is confidence is broken cancelled after first zone. Tests won't run. Version version0 = app.application().oldestDeployedPlatform().get(); - Version version1 = Version.fromString("7.7"); + Version version1 = Version.fromString("6.7"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); @@ -1385,7 +1384,7 @@ public class DeploymentTriggerTest { assertEquals(List.of(), tester.jobs().active()); tester.atMondayMorning().clock().advance(Duration.ofDays(5)); // Inside revision block window for second, conservative instance. - Version version = Version.fromString("8.1"); + Version version = Version.fromString("6.2"); tester.controllerTester().upgradeSystem(version); tester.upgrader().maintain(); assertEquals(Change.of(version), app1.instance().change()); @@ -1495,7 +1494,7 @@ public class DeploymentTriggerTest { // Platform rolls through first production zone. var version0 = tester.controller().readSystemVersion(); - var version1 = new Version("7.1"); + var version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest).runJob(productionUsCentral1); @@ -1529,7 +1528,7 @@ public class DeploymentTriggerTest { assertEquals(Change.empty(), app.instance().change()); // New upgrade fails in staging-test, and revision to fix it is submitted. - var version2 = new Version("7.2"); + var version2 = new Version("6.3"); tester.controllerTester().upgradeSystem(version2); tester.upgrader().maintain(); app.runJob(systemTest).failDeployment(stagingTest); @@ -1561,7 +1560,7 @@ public class DeploymentTriggerTest { // Platform rolls through first production zone. var version0 = tester.controller().readSystemVersion(); - var version1 = new Version("7.1"); + var version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest).runJob(productionUsEast3); @@ -1611,7 +1610,7 @@ public class DeploymentTriggerTest { // Platform rolls through first production zone. var version0 = tester.controller().readSystemVersion(); - var version1 = new Version("7.1"); + var version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest).runJob(productionUsEast3); @@ -1837,8 +1836,8 @@ public class DeploymentTriggerTest { // A version releases, but when the first upgrade has gotten through alpha, beta, and gamma, a newer version has high confidence. var version0 = tester.controller().readSystemVersion(); - var version1 = new Version("7.1"); - var version2 = new Version("7.2"); + var version1 = new Version("6.2"); + var version2 = new Version("6.3"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); @@ -1869,7 +1868,7 @@ public class DeploymentTriggerTest { // Platform rolls through first production zone. var version0 = tester.controller().readSystemVersion(); - var version1 = new Version("7.1"); + var version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest).runJob(productionUsCentral1); @@ -1914,7 +1913,7 @@ public class DeploymentTriggerTest { // Platform rolls through first production zone. var version0 = tester.controller().readSystemVersion(); - var version1 = new Version("7.1"); + var version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); app.runJob(systemTest).runJob(stagingTest).runJob(productionUsCentral1); @@ -1979,7 +1978,7 @@ public class DeploymentTriggerTest { // Manually deploy to east again, then upgrade the system. app.runJob(productionUsEast3, cdPackage); - var version = new Version("7.1"); + var version = new Version("6.2"); tester.controllerTester().upgradeSystem(version); tester.upgrader().maintain(); // System and staging tests both require unknown versions, and are broken. @@ -2039,11 +2038,11 @@ public class DeploymentTriggerTest { conservative.runJob(productionEuWest1) .runJob(testEuWest1); - tester.controllerTester().upgradeSystem(new Version("7.7.7")); + tester.controllerTester().upgradeSystem(new Version("6.7.7")); tester.upgrader().maintain(); canary.runJob(systemTest) - .runJob(stagingTest); + .runJob(stagingTest); tester.upgrader().maintain(); conservative.runJob(productionEuWest1) .runJob(testEuWest1); @@ -2055,7 +2054,7 @@ public class DeploymentTriggerTest { var app = tester.newDeploymentContext().submit().deploy(); // Start upgrade, then receive new submission. - Version version1 = new Version("7.8.9"); + Version version1 = new Version("6.8.9"); RevisionId build1 = app.lastSubmission().get(); tester.controllerTester().upgradeSystem(version1); tester.upgrader().maintain(); @@ -2104,8 +2103,8 @@ public class DeploymentTriggerTest { var app = tester.newDeploymentContext().submit(applicationPackage).deploy(); var appToAvoidVersionGC = tester.newDeploymentContext("g", "c", "default").submit().deploy(); - Version version2 = new Version("7.8.9"); - Version version3 = new Version("8.9.10"); + Version version2 = new Version("6.8.9"); + Version version3 = new Version("6.9.10"); tester.controllerTester().upgradeSystem(version2); tester.deploymentTrigger().forceChange(appToAvoidVersionGC.instanceId(), Change.of(version2)); appToAvoidVersionGC.deployPlatform(version2); @@ -2256,10 +2255,9 @@ public class DeploymentTriggerTest { ApplicationPackage appPackage = ApplicationPackageBuilder.fromDeploymentXml(spec); DeploymentContext tests = tester.newDeploymentContext("tenant", "application", "tests"); DeploymentContext main = tester.newDeploymentContext("tenant", "application", "main"); - Version version1 = new Version("7"); + Version version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); tests.submit(appPackage); - Optional<RevisionId> revision1 = tests.lastSubmission(); JobId systemTestJob = new JobId(tests.instanceId(), systemTest); JobId stagingTestJob = new JobId(tests.instanceId(), stagingTest); JobId mainJob = new JobId(main.instanceId(), productionUsEast3); @@ -2286,10 +2284,10 @@ public class DeploymentTriggerTest { assertEquals(Set.of(), tests.deploymentStatus().jobsToRun().keySet()); // Versions 2 and 3 become available. - // Tests instance fails on 2, then update to 3. + // Tests instance fails on 2, then updates to 3. // Version 2 should not be a target for either instance. // Version 2 should also not be possible to set as a forced target for the tests instance. - Version version2 = new Version("8"); + Version version2 = new Version("6.3"); tester.controllerTester().upgradeSystem(version2); tester.upgrader().run(); tester.triggerJobs(); @@ -2299,7 +2297,7 @@ public class DeploymentTriggerTest { assertEquals(Set.of(systemTestJob), tests.deploymentStatus().jobsToRun().keySet()); assertEquals(2, tests.deploymentStatus().jobsToRun().get(systemTestJob).size()); - Version version3 = new Version("9"); + Version version3 = new Version("6.4"); tester.controllerTester().upgradeSystem(version3); tests.runJob(systemTest) // Success in default cloud. .failDeployment(systemTest); // Failure in centauri cloud. @@ -2417,7 +2415,7 @@ public class DeploymentTriggerTest { existing.add(ZoneApiMock.newBuilder().withCloud("pink-clouds").withId("test.zone").build()); zones.setZones(existing); - JobType defaultSystemTest = JobType.systemTest(zones, CloudName.defaultName()); + JobType defaultSystemTest = JobType.systemTest(zones, CloudName.DEFAULT); JobType pinkSystemTest = JobType.systemTest(zones, CloudName.from("pink-clouds")); // Job name is identity, used for looking up run history, etc.. diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java index 4bb35d748db..528ef6d6192 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java @@ -86,7 +86,7 @@ public class ZoneApiMock implements ZoneApi { private SystemName systemName = SystemName.defaultSystem(); private ZoneId id = ZoneId.defaultId(); private ZoneId virtualId = null; - private CloudName cloudName = CloudName.defaultName(); + private CloudName cloudName = CloudName.DEFAULT; private String cloudNativeRegionName = id.region().value(); public Builder with(ZoneId id) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java index 6280725794c..e79793bab61 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java @@ -23,7 +23,7 @@ public class ArtifactExpirerTest { void maintain() { DeploymentTester tester = new DeploymentTester(); ArtifactExpirer expirer = new ArtifactExpirer(tester.controller(), Duration.ofDays(1)); - ArtifactRegistryMock registry = tester.controllerTester().serviceRegistry().artifactRegistry(CloudName.defaultName()).orElseThrow(); + ArtifactRegistryMock registry = tester.controllerTester().serviceRegistry().artifactRegistry(CloudName.DEFAULT).orElseThrow(); Instant instant = tester.clock().instant(); Artifact image0 = new Artifact("image0", "registry.example.com", "vespa/vespa", "7.1", instant, Version.fromString("7.1")); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java index b41c17fcd33..64baa107a1c 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java @@ -218,11 +218,11 @@ public class MetricsReporterTest { assertFalse(context.instance().change().hasTargets(), "Change deployed"); // New versions is released and upgrade fails in test environments - Version version = Version.fromString("7.1"); + Version version = Version.fromString("6.2"); tester.controllerTester().upgradeSystem(version); tester.upgrader().maintain(); context.failDeployment(systemTest) - .failDeployment(stagingTest); + .failDeployment(stagingTest); reporter.maintain(); assertEquals(2, getDeploymentsFailingUpgrade(context.instanceId())); @@ -356,9 +356,9 @@ public class MetricsReporterTest { var tester = new ControllerTester(); var reporter = createReporter(tester.controller()); var zone = ZoneId.from("prod.eu-west-1"); - var cloud = CloudName.defaultName(); + var cloud = CloudName.DEFAULT; tester.zoneRegistry().setOsUpgradePolicy(cloud, UpgradePolicy.builder().upgrade(ZoneApiMock.from(zone)).build()); - var osUpgrader = new OsUpgrader(tester.controller(), Duration.ofDays(1), CloudName.defaultName()); + var osUpgrader = new OsUpgrader(tester.controller(), Duration.ofDays(1), CloudName.DEFAULT); var statusUpdater = new OsVersionStatusUpdater(tester.controller(), Duration.ofDays(1) ); tester.configServer().bootstrap(List.of(zone), SystemApplication.configServerHost, SystemApplication.tenantHost); @@ -529,7 +529,7 @@ public class MetricsReporterTest { context.submit(pkg).completeRollout(); // System is upgraded, triggering upgrade of application - tester.controllerTester().upgradeSystem(Version.fromString("7.0")); + tester.controllerTester().upgradeSystem(Version.fromString("6.2")); tester.upgrader().maintain(); // Start production job for upgrade, without completing it diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java index 0bd3810e9a4..2608a722e49 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java @@ -32,14 +32,14 @@ public class OsVersionStatusUpdaterTest { for (ZoneApi zone : tester.zoneRegistry().zones().controllerUpgraded().zones()) { upgradePolicy = upgradePolicy.upgrade(zone); } - tester.zoneRegistry().setOsUpgradePolicy(CloudName.defaultName(), upgradePolicy.build()); + tester.zoneRegistry().setOsUpgradePolicy(CloudName.DEFAULT, upgradePolicy.build()); // Initially empty assertSame(OsVersionStatus.empty, tester.controller().osVersionStatus()); // Setting a new target adds it to current status Version version1 = Version.fromString("7.1"); - CloudName cloud = CloudName.defaultName(); + CloudName cloud = CloudName.DEFAULT; tester.controller().upgradeOsIn(cloud, version1, Duration.ZERO, false); statusUpdater.maintain(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java index b24b6c8e99b..76f0cacb7c5 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java @@ -16,6 +16,7 @@ import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger; import com.yahoo.vespa.hosted.controller.deployment.Run; import com.yahoo.vespa.hosted.controller.versions.VespaVersion; +import com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence; import org.junit.jupiter.api.Test; import java.time.Duration; @@ -663,116 +664,49 @@ public class UpgraderTest { Version version = Version.fromString("6.2"); tester.controllerTester().upgradeSystem(version); - ApplicationPackage version6ApplicationPackage = new ApplicationPackageBuilder().majorVersion(6) - .region("us-west-1") - .build(); + ApplicationPackageBuilder builder = new ApplicationPackageBuilder().region("us-west-1").majorVersion(7); + ApplicationPackage defaultPackage = new ApplicationPackageBuilder().region("us-west-1").build(); // Setup applications - var canary0 = createAndDeploy("canary0", "canary"); - var default0 = tester.newDeploymentContext().submit(version6ApplicationPackage).deploy(); + var canaryApp = tester.newDeploymentContext("canary", "app", "default").submit(builder.upgradePolicy("canary").build()).deploy(); + var defaultApp = tester.newDeploymentContext("normal", "app", "default").submit(builder.upgradePolicy("default").build()).deploy(); + var conservativeApp = tester.newDeploymentContext("conservative", "app", "default").submit(builder.upgradePolicy("conservative").build()).deploy(); + var lazyApp = tester.newDeploymentContext().submit(defaultPackage).deploy(); - // New major version is released + // New major version is released; more apps upgrade with increasing confidence. version = Version.fromString("7.0"); tester.controllerTester().upgradeSystem(version); - tester.upgrader().maintain(); - assertEquals(version, tester.controller().readVersionStatus().systemVersion().get().versionNumber()); - tester.triggerJobs(); - - // ... canary upgrade to it - assertEquals(2, tester.jobs().active().size()); - canary0.deployPlatform(version); - assertEquals(0, tester.jobs().active().size()); + tester.upgrader().overrideConfidence(version, Confidence.broken); tester.controllerTester().computeVersionStatus(); - - // The other application does not because it has pinned to major version 6 - tester.upgrader().maintain(); - tester.triggerJobs(); - assertEquals(0, tester.jobs().active().size()); - } - - @Test - void testPinningMajorVersionInApplication() { - Version version = Version.fromString("6.2"); - tester.controllerTester().upgradeSystem(version); - - // Setup applications - var canary0 = createAndDeploy("canary", "canary"); - var default0 = tester.newDeploymentContext().submit().deploy(); - tester.applications().lockApplicationOrThrow(default0.application().id(), - a -> tester.applications().store(a.withMajorVersion(6))); - assertEquals(OptionalInt.of(6), default0.application().majorVersion()); - - // New major version is released - version = Version.fromString("7.0"); - tester.controllerTester().upgradeSystem(version); - assertEquals(version, tester.controller().readVersionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); - tester.triggerJobs(); + assertEquals(Change.of(version), canaryApp.instance().change()); + assertEquals(Change.empty(), defaultApp.instance().change()); + assertEquals(Change.empty(), conservativeApp.instance().change()); + assertEquals(Change.empty(), lazyApp.instance().change()); - // ... canary upgrade to it - assertEquals(2, tester.jobs().active().size()); - canary0.deployPlatform(version); - assertEquals(0, tester.jobs().active().size()); + tester.upgrader().overrideConfidence(version, Confidence.low); tester.controllerTester().computeVersionStatus(); - - // The other application does not because it has pinned to major version 6 tester.upgrader().maintain(); - tester.triggerJobs(); - assertEquals(0, tester.jobs().active().size()); - } - - @Test - void testPinningMajorVersionInUpgrader() { - Version version = Version.fromString("6.2"); - tester.controllerTester().upgradeSystem(version); - - ApplicationPackage version7CanaryApplicationPackage = new ApplicationPackageBuilder() - .majorVersion(7) - .upgradePolicy("canary") - .region("us-west-1") - .build(); - ApplicationPackage version7DefaultApplicationPackage = new ApplicationPackageBuilder() - .majorVersion(7) - .upgradePolicy("default") - .region("us-west-1") - .build(); - - // Setup applications - var canary0 = tester.newDeploymentContext("tenant1", "canary0", "default").submit(version7CanaryApplicationPackage).deploy(); - var default0 = tester.newDeploymentContext("tenant1", "default0", "default").submit(version7DefaultApplicationPackage).deploy(); - var default1 = tester.newDeploymentContext("tenant1", "default1", "default").submit(DeploymentContext.applicationPackage()).deploy(); + assertEquals(Change.of(version), canaryApp.instance().change()); + assertEquals(Change.empty(), defaultApp.instance().change()); + assertEquals(Change.empty(), conservativeApp.instance().change()); + assertEquals(Change.empty(), lazyApp.instance().change()); - // New major version is released, but we don't want to upgrade to it yet - tester.upgrader().setTargetMajorVersion(OptionalInt.of(6)); - version = Version.fromString("7.0"); - tester.controllerTester().upgradeSystem(version); - assertEquals(version, tester.controller().readVersionStatus().systemVersion().get().versionNumber()); - tester.upgrader().maintain(); - tester.triggerJobs(); - - // ... canary upgrade to it because it explicitly wants 7 - assertEquals(2, tester.jobs().active().size()); - canary0.deployPlatform(version); - assertEquals(0, tester.jobs().active().size()); + tester.upgrader().overrideConfidence(version, Confidence.normal); tester.controllerTester().computeVersionStatus(); - - // default0 upgrades, but not default1 tester.upgrader().maintain(); - tester.triggerJobs(); - assertEquals(2, tester.jobs().active().size()); - default0.deployPlatform(version); + assertEquals(Change.of(version), canaryApp.instance().change()); + assertEquals(Change.of(version), defaultApp.instance().change()); + assertEquals(Change.empty(), conservativeApp.instance().change()); + assertEquals(Change.empty(), lazyApp.instance().change()); - // Nothing more happens ... - tester.upgrader().maintain(); - tester.triggerJobs(); - assertEquals(0, tester.jobs().active().size()); - - // Now we want to upgrade the latest application - tester.upgrader().setTargetMajorVersion(OptionalInt.empty()); + tester.upgrader().overrideConfidence(version, Confidence.high); + tester.controllerTester().computeVersionStatus(); tester.upgrader().maintain(); - tester.triggerJobs(); - assertEquals(2, tester.jobs().active().size()); - default1.deployPlatform(version); + assertEquals(Change.of(version), canaryApp.instance().change()); + assertEquals(Change.of(version), defaultApp.instance().change()); + assertEquals(Change.of(version), conservativeApp.instance().change()); + assertEquals(Change.empty(), lazyApp.instance().change()); } @Test @@ -955,9 +889,6 @@ public class UpgraderTest { Version version0 = Version.fromString("6.1"); tester.controllerTester().upgradeSystem(version0); - // Apps target 6 by default - tester.upgrader().setTargetMajorVersion(OptionalInt.of(6)); - // All applications deploy on current version var app1 = createAndDeploy("app1", "default"); var app2 = createAndDeploy("app2", "default"); @@ -979,10 +910,10 @@ public class UpgraderTest { Version version2 = Version.fromString("7.1"); tester.controllerTester().upgradeSystem(version2); - // App 2 is allowed on new major and upgrades - tester.controller().applications().lockApplicationIfPresent(app2.application().id(), app -> tester.applications().store(app.withMajorVersion(7))); + // App 2 has upgrade to new platform triggered + tester.deploymentTrigger().forceChange(app2.instanceId(), Change.of(version2)); tester.upgrader().maintain(); - assertEquals(version2, app2.instance().change().platform().orElseThrow()); + assertEquals(Change.of(version2), app2.instance().change()); // App 1 is unpinned and upgrades to latest 6 tester.controller().applications().lockApplicationIfPresent(app1.application().id(), app -> @@ -1004,7 +935,7 @@ public class UpgraderTest { app.failDeployment(systemTest); // New version is not targeted. - Version version1 = new Version("7"); + Version version1 = new Version("6.2"); tester.controllerTester().upgradeSystem(version1); assertEquals(Change.of(revision1.get()), app.instance().change()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java index 568b17817a1..7f988f08a89 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java @@ -20,7 +20,7 @@ public class OsVersionSerializerTest { void test_serialization() { OsVersionSerializer serializer = new OsVersionSerializer(); Set<OsVersion> osVersions = ImmutableSet.of( - new OsVersion(Version.fromString("7.1"), CloudName.defaultName()), + new OsVersion(Version.fromString("7.1"), CloudName.DEFAULT), new OsVersion(Version.fromString("7.1"), CloudName.from("foo")) ); Set<OsVersion> serialized = serializer.fromSlime(serializer.toSlime(osVersions)); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java index 7461bf6516c..e552dfe94e1 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java @@ -29,11 +29,11 @@ public class OsVersionStatusSerializerTest { Version version2 = Version.fromString("7.2"); Map<OsVersion, List<NodeVersion>> versions = new LinkedHashMap<>(); - versions.put(new OsVersion(version1, CloudName.defaultName()), List.of( + versions.put(new OsVersion(version1, CloudName.DEFAULT), List.of( new NodeVersion(HostName.of("node1"), ZoneId.from("prod", "us-west"), version1, version2, Optional.of(Instant.ofEpochMilli(11))), new NodeVersion(HostName.of("node2"), ZoneId.from("prod", "us-east"), version1, version2, Optional.of(Instant.ofEpochMilli(22))) )); - versions.put(new OsVersion(version2, CloudName.defaultName()), List.of( + versions.put(new OsVersion(version2, CloudName.DEFAULT), List.of( new NodeVersion(HostName.of("node3"), ZoneId.from("prod", "us-west"), version2, version2, Optional.of(Instant.ofEpochMilli(33))), new NodeVersion(HostName.of("node4"), ZoneId.from("prod", "us-east"), version2, version2, Optional.of(Instant.ofEpochMilli(44))) )); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java index 654703b36c0..0f8a1c1b056 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java @@ -23,7 +23,7 @@ public class OsVersionTargetSerializerTest { void serialization() { OsVersionTargetSerializer serializer = new OsVersionTargetSerializer(new OsVersionSerializer()); Set<OsVersionTarget> targets = ImmutableSet.of( - new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.defaultName()), Duration.ZERO, Instant.ofEpochMilli(123)), + new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.DEFAULT), Duration.ZERO, Instant.ofEpochMilli(123)), new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.from("foo")), Duration.ofDays(1), Instant.ofEpochMilli(456)) ); Set<OsVersionTarget> serialized = serializer.fromSlime(serializer.toSlime(targets)); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java index fa652af18f3..9148ad36d46 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java @@ -93,18 +93,6 @@ public class ControllerApiTest extends ControllerContainerTest { "{\"upgradesPerMinute\":42.0,\"confidenceOverrides\":[]}", 200); - // Set target major version - tester.assertResponse( - operatorRequest("http://localhost:8080/controller/v1/jobs/upgrader", "{\"targetMajorVersion\":6}", Request.Method.PATCH), - "{\"upgradesPerMinute\":42.0,\"targetMajorVersion\":6,\"confidenceOverrides\":[]}", - 200); - - // Clear target major version - tester.assertResponse( - operatorRequest("http://localhost:8080/controller/v1/jobs/upgrader", "{\"targetMajorVersion\":null}", Request.Method.PATCH), - "{\"upgradesPerMinute\":42.0,\"confidenceOverrides\":[]}", - 200); - // Override confidence tester.assertResponse( operatorRequest("http://localhost:8080/controller/v1/jobs/upgrader/confidence/6.42", "broken", Request.Method.POST), diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java index 18776be2dce..6cb7c059b1d 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java @@ -384,7 +384,6 @@ public class VersionStatusTest { // A new major version is released and all canaries upgrade Version version4 = new Version("7.1"); - tester.controller().applications().setTargetMajorVersion(OptionalInt.of(version3.getMajor())); // Previous remains the default tester.controllerTester().upgradeSystem(version4); tester.upgrader().maintain(); tester.triggerJobs(); 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 90830b04124..2cf9ff94b35 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -495,6 +495,13 @@ public class Flags { "Takes effect at redeployment", ZONE_ID, APPLICATION_ID); + public static final UnboundIntFlag CLUSTER_CONTROLLER_STATE_GATHER_COUNT = defineIntFlag( + "cluster-controller-state-gather-count", 2, + List.of("hmusum"), "2022-09-05", "2022-11-01", + "Count of how many cluster controllers should gather node state (range: [1, # of cluster controllers])", + "Takes effect at redeployment", + ZONE_ID, APPLICATION_ID); + /** WARNING: public for testing: All flags should be defined in {@link Flags}. */ public static UnboundBooleanFlag defineFeatureFlag(String flagId, boolean defaultValue, List<String> owners, String createdAt, String expiresAt, String description, diff --git a/integration/intellij/build.gradle b/integration/intellij/build.gradle index 535911c5b8e..8f8c71ad5c7 100644 --- a/integration/intellij/build.gradle +++ b/integration/intellij/build.gradle @@ -36,7 +36,7 @@ compileJava { } group 'ai.vespa' -version '1.2.0' // Also update pom.xml version if this is changed +version '1.3.0' // Also update pom.xml version if this is changed sourceCompatibility = 11 @@ -61,10 +61,10 @@ buildSearchableOptions { patchPluginXml { version = project.version sinceBuild = '203' - untilBuild = '213.*' + untilBuild = '222.*' // in changeNotes you can add a description of the changes in this version (would appear in the plugin page in preferences\plugins) changeNotes = """ - Complete grammar (no more red squiggles) + Support for IntelliJ 2022 """ } diff --git a/integration/intellij/pom.xml b/integration/intellij/pom.xml index d7146a60f7a..841c2ad7ee1 100644 --- a/integration/intellij/pom.xml +++ b/integration/intellij/pom.xml @@ -9,7 +9,7 @@ <relativePath>../parent/pom.xml</relativePath> </parent> <artifactId>vespa-intellij</artifactId> <!-- Not used - plugin is build by gradle --> - <version>1.2.0</version> <!-- See copy-zip below, which depends on this being the same as the v. in build.gradle --> + <version>1.3.0</version> <!-- See copy-zip below, which depends on this being the same as the v. in build.gradle --> <description> Maven wrapper for the gradle build of this IntelliJ plugin. </description> diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java index b526c573c05..d942e5c9e80 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/servicedump/VespaServiceDumperImpl.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.node.admin.maintenance.servicedump; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.CloudName; import com.yahoo.text.Lowercase; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeAttributes; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository; @@ -61,7 +62,7 @@ public class VespaServiceDumperImpl implements VespaServiceDumper { @Override public void processServiceDumpRequest(NodeAgentContext context) { - if (context.zone().getCloudName().value().equals("gcp")) return; + if (context.zone().getCloudName().equals(CloudName.GCP)) return; Instant startedAt = clock.instant(); NodeSpec nodeSpec = context.node(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java index 47c96103ab5..ed2de691eb0 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java @@ -262,7 +262,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { @Override public CloudName getCloudName() { - return CloudName.defaultName(); + return CloudName.DEFAULT; } @Override diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java index a9e7ded66e6..dcf39cd5ec4 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java @@ -137,7 +137,7 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer { if (host.state() != mutex.node().state()) return; host = mutex.node(); // First mark the host as wantToDeprovision so that if hostProvisioner fails, this host - // * wont get new nodes allocated to it + // * won't get new nodes allocated to it // * will be selected as excess on next iteration of this maintainer nodeRepository().nodes().deprovision(host.hostname(), Agent.DynamicProvisioningMaintainer, nodeRepository().clock().instant()); hostProvisioner.deprovision(host); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java index 5237e48907a..947a000eecf 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java @@ -127,14 +127,14 @@ public class CapacityPolicies { // The lowest amount resources that can be exclusive allocated (i.e. a matching host flavor for this exists) private NodeResources smallestExclusiveResources() { - return (zone.getCloud().name().equals(CloudName.from("gcp"))) + return (zone.getCloud().name().equals(CloudName.GCP)) ? new NodeResources(1, 4, 50, 0.3) : new NodeResources(0.5, 4, 50, 0.3); } // The lowest amount resources that can be shared (i.e. a matching host flavor for this exists) private NodeResources smallestSharedResources() { - return (zone.getCloud().name().equals(CloudName.from("gcp"))) + return (zone.getCloud().name().equals(CloudName.GCP)) ? new NodeResources(1, 4, 50, 0.3) : new NodeResources(0.5, 2, 50, 0.3); } diff --git a/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp b/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp index 0ce67e8bfcb..4ff459a5a9d 100644 --- a/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp +++ b/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp @@ -33,6 +33,14 @@ ostream &operator<<(ostream &os, const SummarymapConfig::Override &override) { } +namespace vespa::config::search::internal { + +std::ostream &operator<<(std::ostream &os, const SummaryConfig::Classes::Fields &field) { + return os << "{name=" << field.name << ", type=" << field.type << ", command=" << field.command << ", source=" << field.source << "}"; +} + +} + namespace proton { namespace { @@ -159,6 +167,16 @@ SummaryConfig::Classes::Fields make_summary_field(const vespalib::string &name, return field; } +SummaryConfig::Classes::Fields make_summary_field(const vespalib::string &name, const vespalib::string &type, const vespalib::string& command, const vespalib::string& source) +{ + SummaryConfig::Classes::Fields field; + field.name = name; + field.type = type; + field.command = command; + field.source = source; + return field; +} + SummaryConfig sCfg(std::vector<SummaryConfig::Classes::Fields> fields) { SummaryConfigBuilder result; @@ -235,6 +253,12 @@ public: auto summarymapConfig = _delayer.getSummarymapConfig(); EXPECT_EQ(exp, summarymapConfig->override); } + void assertSummaryConfig(const std::vector<SummaryConfig::Classes::Fields> &exp) + { + auto summaryConfig = _delayer.getSummaryConfig(); + ASSERT_EQ(1, summaryConfig->classes.size()); + EXPECT_EQ(exp, summaryConfig->classes[0].fields); + } }; TEST_F(DelayerTest, require_that_empty_config_is_ok) @@ -242,44 +266,50 @@ TEST_F(DelayerTest, require_that_empty_config_is_ok) setup(attrCfg({}), smCfg({}), attrCfg({}), sCfg({}), smCfg({})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({}); } TEST_F(DelayerTest, require_that_simple_attribute_config_is_ok) { - setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer")}), smCfg({make_attribute_override("a")})); + setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_int32_sv_cfg()}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "integer", "attribute", "a")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_is_delayed_if_field_type_is_unchanged) { addFields({"a"}); - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer")}), smCfg({make_attribute_override("a")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a", "integer")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_is_delayed_if_field_type_is_unchanged_geopos_override) { addFields({"a"}); - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer")}), smCfg({make_geopos_override("a")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer", "geopos", "a")}), smCfg({make_geopos_override("a")})); assertAttributeConfig({}); assertSummarymapConfig({make_geopos_override("a")}); + assertSummaryConfig({make_summary_field("a", "integer", "geopos", "a")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_is_delayed_if_field_type_is_unchanged_mapped_summary) { addFields({"a"}); - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a_mapped", "integer")}), smCfg({make_attribute_override("a_mapped", "a")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a_mapped", "integer", "attribute", "a")}), smCfg({make_attribute_override("a_mapped", "a")})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a_mapped", "integer", "copy", "a")}); } TEST_F(DelayerTest, require_that_adding_attribute_is_not_delayed_if_field_type_changed) { - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer")}), smCfg({make_attribute_override("a")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_int32_sv_cfg()}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "integer", "attribute", "a")}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_if_field_type_is_unchanged) @@ -288,6 +318,7 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_if_field_t setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({}), sCfg({make_summary_field("a", "integer")}), smCfg({})); assertAttributeConfig({make_int32_sv_cfg()}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "integer", "attribute", "a")}); } TEST_F(DelayerTest, require_that_summary_map_override_is_removed_when_summary_aspect_is_removed_even_if_removing_attribute_aspect_is_delayed) @@ -296,6 +327,7 @@ TEST_F(DelayerTest, require_that_summary_map_override_is_removed_when_summary_as setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({}), sCfg({}), smCfg({})); assertAttributeConfig({make_int32_sv_cfg()}); assertSummarymapConfig({}); + assertSummaryConfig({}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_if_field_type_is_unchanged_gepos_override) @@ -304,6 +336,7 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_if_field_t setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_geopos_override("a")}), attrCfg({}), sCfg({}), smCfg({})); assertAttributeConfig({make_int32_sv_cfg()}); assertSummarymapConfig({}); + assertSummaryConfig({}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_if_field_type_changed) @@ -311,6 +344,7 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_if_fie setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({}), sCfg({make_summary_field("a", "integer")}), smCfg({})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a", "integer")}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_if_also_indexed) @@ -320,15 +354,17 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_if_als setup(attrCfg({make_string_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({}), sCfg({make_summary_field("a", "string")}), smCfg({})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a", "string")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_is_delayed_for_tensor_field) { addFields({"a"}); setup(attrCfg({}), smCfg({}), - attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")})); + attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a", "tensor")}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_for_tensor_field) @@ -338,6 +374,7 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_for_tensor attrCfg({}), sCfg({make_summary_field("a", "tensor")}), smCfg({})); assertAttributeConfig({make_tensor_cfg("tensor(x[10])")}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "tensor", "attribute", "a")}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_for_predicate) @@ -346,6 +383,7 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_for_pr setup(attrCfg({make_predicate_cfg(4)}), smCfg({}), attrCfg({}), sCfg({make_summary_field("a", "string")}), smCfg({})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a", "string")}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_for_reference) @@ -354,64 +392,72 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_for_re setup(attrCfg({make_reference_cfg()}), smCfg({}), attrCfg({}), sCfg({make_summary_field("a", "longstring")}), smCfg({})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("a", "longstring")}); } TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_false_true_edge) { addFields({"a"}); - setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({make_fa(make_int32_sv_cfg())}), sCfg({make_summary_field("a", "integer")}), smCfg({make_attribute_override("a")})); + setup(attrCfg({make_int32_sv_cfg()}), smCfg({make_attribute_override("a")}), attrCfg({make_fa(make_int32_sv_cfg())}), sCfg({make_summary_field("a", "integer", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_int32_sv_cfg()}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "integer", "attribute", "a")}); } TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_true_false_edge) { addFields({"a"}); - setup(attrCfg({make_fa(make_int32_sv_cfg())}), smCfg({make_attribute_override("a")}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer")}), smCfg({make_attribute_override("a")})); + setup(attrCfg({make_fa(make_int32_sv_cfg())}), smCfg({make_attribute_override("a")}), attrCfg({make_int32_sv_cfg()}), sCfg({make_summary_field("a", "integer", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_fa(make_int32_sv_cfg())}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "integer", "attribute", "a")}); } TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_false_true_edge_on_tensor_attribute) { addFields({"a"}); setup(attrCfg({make_tensor_cfg("tensor(x[10])")}), smCfg({make_attribute_override("a")}), - attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")})); + attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), sCfg({make_summary_field("a", "tensor", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_tensor_cfg("tensor(x[10])")}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "tensor", "attribute", "a")}); } TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_true_false_edge_on_tensor_attribute) { addFields({"a"}); setup(attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), smCfg({make_attribute_override("a")}), - attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")})); + attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_fa(make_tensor_cfg("tensor(x[10])"))}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "tensor", "attribute", "a")}); } TEST_F(DelayerTest, require_that_fast_access_flag_change_is_not_delayed_true_false_edge_on_string_attribute_indexed_field) { addFields({"a"}); addOldIndexField("a"); - setup(attrCfg({make_fa(make_string_sv_cfg())}), smCfg({make_attribute_override("a")}), attrCfg({make_string_sv_cfg()}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")})); + setup(attrCfg({make_fa(make_string_sv_cfg())}), smCfg({make_attribute_override("a")}), attrCfg({make_string_sv_cfg()}), sCfg({make_summary_field("a", "string", "attribute", "a")}), smCfg({make_attribute_override("a")})); assertAttributeConfig({make_string_sv_cfg()}); assertSummarymapConfig({make_attribute_override("a")}); + assertSummaryConfig({make_summary_field("a", "string", "attribute", "a")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_to_struct_field_is_not_delayed_if_field_type_is_changed) { - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), sCfg({make_summary_field("array", "jsonstring")}), smCfg({make_attribute_combiner_override("array")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), sCfg({make_summary_field("array", "jsonstring", "attributecombiner", "array")}), smCfg({make_attribute_combiner_override("array")})); assertAttributeConfig({make_int32_sv_cfg("array.a")}); assertSummarymapConfig({make_attribute_combiner_override("array")}); + assertSummaryConfig({make_summary_field("array", "jsonstring", "attributecombiner", "array")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_to_struct_field_is_delayed_if_field_type_is_unchanged) { addFields({"array.a"}); - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), sCfg({make_summary_field("array", "jsonstring")}), smCfg({make_attribute_combiner_override("array")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), sCfg({make_summary_field("array", "jsonstring", "attributecombiner", "array")}), smCfg({make_attribute_combiner_override("array")})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("array", "jsonstring")}); } TEST_F(DelayerTest, require_that_removing_attribute_aspect_from_struct_field_is_not_delayed) @@ -420,14 +466,16 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_from_struct_field_is_ setup(attrCfg({make_int32_sv_cfg("array.a")}), smCfg({make_attribute_combiner_override("array")}), attrCfg({}), sCfg({make_summary_field("array", "jsonstring")}), smCfg({})); assertAttributeConfig({}); assertSummarymapConfig({}); + assertSummaryConfig({make_summary_field("array", "jsonstring")}); } TEST_F(DelayerTest, require_that_adding_attribute_aspect_to_struct_field_is_delayed_if_field_type_is_unchanged_with_filtering_docsum) { addFields({"array.a"}); - setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), sCfg({make_summary_field("array", "jsonstring")}), smCfg({make_attribute_combiner_override("array"), make_matched_attribute_elements_filter_override("array_filtered", "array")})); + setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), sCfg({make_summary_field("array", "jsonstring", "attributecombiner", "array"), make_summary_field("array_filtered", "jsonstring", "matchedattributeelementsfilter", "array")}), smCfg({make_attribute_combiner_override("array"), make_matched_attribute_elements_filter_override("array_filtered", "array")})); assertAttributeConfig({}); assertSummarymapConfig({make_matched_elements_filter_override("array_filtered", "array")}); + assertSummaryConfig({make_summary_field("array", "jsonstring"), make_summary_field("array_filtered", "jsonstring", "matchedelementsfilter", "array")}); } } diff --git a/searchcore/src/tests/proton/docsummary/docsummary.cpp b/searchcore/src/tests/proton/docsummary/docsummary.cpp index 93e8f3d248b..b034b77086c 100644 --- a/searchcore/src/tests/proton/docsummary/docsummary.cpp +++ b/searchcore/src/tests/proton/docsummary/docsummary.cpp @@ -35,6 +35,7 @@ #include <vespa/searchlib/tensor/tensor_attribute.h> #include <vespa/searchlib/transactionlog/nosyncproxy.h> #include <vespa/searchlib/transactionlog/translogserver.h> +#include <vespa/searchsummary/docsummary/i_docsum_field_writer_factory.h> #include <vespa/searchsummary/docsummary/i_docsum_store_document.h> #include <vespa/searchsummary/docsummary/summaryfieldconverter.h> #include <vespa/vespalib/data/simple_buffer.h> @@ -77,6 +78,19 @@ using namespace vespalib::slime; namespace proton { +class MockDocsumFieldWriterFactory : public search::docsummary::IDocsumFieldWriterFactory +{ +public: + std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc) override { + (void) fieldName; + (void) overrideName; + (void) argument; + (void) rc; + return {}; + } + +}; + class DirMaker { public: @@ -1121,7 +1135,8 @@ Fixture::Fixture() std::string cfgId("summary"); _summaryCfg = ConfigGetter<vespa::config::search::SummaryConfig>::getConfig( cfgId, ::config::FileSpec(TEST_PATH("summary.cfg"))); - _resultCfg.ReadConfig(*_summaryCfg, cfgId.c_str()); + auto docsum_field_writer_factory = std::make_unique<MockDocsumFieldWriterFactory>(); + _resultCfg.ReadConfig(*_summaryCfg, cfgId.c_str(), *docsum_field_writer_factory); } Fixture::~Fixture() = default; diff --git a/searchcore/src/tests/proton/docsummary/summary.cfg b/searchcore/src/tests/proton/docsummary/summary.cfg index cceb15c8fd2..33641351c02 100644 --- a/searchcore/src/tests/proton/docsummary/summary.cfg +++ b/searchcore/src/tests/proton/docsummary/summary.cfg @@ -61,31 +61,54 @@ classes[2].id 2 classes[2].fields[2] classes[2].fields[0].name "aa" classes[2].fields[0].type "integer" +classes[2].fields[0].command "copy" +classes[2].fields[0].source "ab" classes[2].fields[1].name "ab" classes[2].fields[1].type "integer" +classes[2].fields[1].command "empty" classes[3].name "class3" classes[3].id 3 classes[3].fields[10] classes[3].fields[0].name "ba" classes[3].fields[0].type "integer" +classes[3].fields[0].command "attribute" +classes[3].fields[0].source "ba" classes[3].fields[1].name "bb" classes[3].fields[1].type "float" +classes[3].fields[1].command "attribute" +classes[3].fields[1].source "bb" classes[3].fields[2].name "bc" classes[3].fields[2].type "longstring" +classes[3].fields[2].command "attribute" +classes[3].fields[2].source "bc" classes[3].fields[3].name "bd" classes[3].fields[3].type "jsonstring" +classes[3].fields[3].command "attribute" +classes[3].fields[3].source "bd" classes[3].fields[4].name "be" classes[3].fields[4].type "jsonstring" +classes[3].fields[4].command "attribute" +classes[3].fields[4].source "be" classes[3].fields[5].name "bf" classes[3].fields[5].type "jsonstring" +classes[3].fields[5].command "attribute" +classes[3].fields[5].source "bf" classes[3].fields[6].name "bg" classes[3].fields[6].type "jsonstring" +classes[3].fields[6].command "attribute" +classes[3].fields[6].source "bg" classes[3].fields[7].name "bh" classes[3].fields[7].type "jsonstring" +classes[3].fields[7].command "attribute" +classes[3].fields[7].source "bh" classes[3].fields[8].name "bi" classes[3].fields[8].type "jsonstring" +classes[3].fields[8].command "attribute" +classes[3].fields[8].source "bi" classes[3].fields[9].name "bj" classes[3].fields[9].type "tensor" +classes[3].fields[9].command "attribute" +classes[3].fields[9].source "bj" classes[4].name "class4" classes[4].id 4 classes[4].fields[1] @@ -98,18 +121,28 @@ classes[5].fields[0].name "sp2" classes[5].fields[0].type "int64" classes[5].fields[1].name "sp2x" classes[5].fields[1].type "xmlstring" +classes[5].fields[1].command "positions" +classes[5].fields[1].source "sp2" classes[5].fields[2].name "ap2" classes[5].fields[2].type "jsonstring" classes[5].fields[3].name "ap2x" classes[5].fields[3].type "xmlstring" +classes[5].fields[3].command "positions" +classes[5].fields[3].source "ap2" classes[5].fields[4].name "wp2" classes[5].fields[4].type "jsonstring" classes[5].fields[5].name "wp2x" classes[5].fields[5].type "xmlstring" +classes[5].fields[5].command "positions" +classes[5].fields[5].source "wp2" classes[6].name "class6" classes[6].id 6 classes[6].fields[6] classes[6].fields[0].name "ba" classes[6].fields[0].type "integer" +classes[6].fields[0].command "attribute" +classes[6].fields[0].source "ba" classes[6].fields[1].name "bb" classes[6].fields[1].type "float" +classes[6].fields[1].command "attribute" +classes[6].fields[1].source "bb" diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp index 6e935a577fd..2b954cf4dac 100644 --- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp +++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp @@ -18,6 +18,7 @@ using search::attribute::ConfigConverter; using vespa::config::search::AttributesConfig; using vespa::config::search::AttributesConfigBuilder; using vespa::config::search::SummaryConfig; +using vespa::config::search::SummaryConfigBuilder; using vespa::config::search::SummarymapConfig; using vespa::config::search::SummarymapConfigBuilder; @@ -25,6 +26,12 @@ namespace proton { namespace { +vespalib::string attribute_combiner_dfw_string("attributecombiner"); +vespalib::string matched_attribute_elements_filter_dfw_string("matchedattributeelementsfilter"); +vespalib::string matched_elements_filter_dfw_string("matchedelementsfilter"); +vespalib::string copy_dfw_string("copy"); +vespalib::string attribute_dfw_string("attribute"); + using AttributesConfigHash = ConfigHash<AttributesConfig::Attribute>; bool willTriggerReprocessOnAttributeAspectRemoval(const search::attribute::Config &cfg, @@ -68,144 +75,296 @@ vespalib::string source_field(const SummarymapConfig::Override &override) { } } +vespalib::string +source_field(const SummaryConfig::Classes::Fields& summary_field) +{ + if (summary_field.source == "") { + return summary_field.name; + } else { + return summary_field.source; + } } -AttributeAspectDelayer::AttributeAspectDelayer() - : _attributesConfig(std::make_shared<AttributesConfigBuilder>()), - _summarymapConfig(std::make_shared<SummarymapConfigBuilder>()) +void +remove_docsum_field_rewriter(SummaryConfig::Classes::Fields& summary_field) { + if (source_field(summary_field) != summary_field.name) { + summary_field.command = copy_dfw_string; + } else { + summary_field.command = ""; + summary_field.source = ""; + } } -AttributeAspectDelayer::~AttributeAspectDelayer() +class AttributeAspectConfigRewriter +{ + const AttributesConfig& _old_attributes_config; + const AttributesConfig& _new_attributes_config; + AttributesConfigHash _old_attributes_config_hash; + AttributesConfigHash _new_attributes_config_hash; + const IIndexschemaInspector& _old_index_schema_inspector; + const IDocumentTypeInspector& _inspector; + vespalib::hash_set<vespalib::string> _delayed_add_attribute_aspect; + vespalib::hash_set<vespalib::string> _delayed_add_attribute_aspect_struct; + vespalib::hash_set<vespalib::string> _delayed_remove_attribute_aspect; + + bool has_unchanged_field(const vespalib::string& name) const; + bool should_delay_add_attribute_aspect(const vespalib::string& name) const; + bool should_delay_remove_attribute_aspect(const vespalib::string& name) const; + bool calculate_fast_access(const AttributesConfig::Attribute& new_attribute_config) const; + void mark_delayed_add_attribute_aspect(const vespalib::string& name) { _delayed_add_attribute_aspect.insert(name); } + bool is_delayed_add_attribute_aspect(const vespalib::string& name) const noexcept { return _delayed_add_attribute_aspect.find(name) != _delayed_add_attribute_aspect.end(); } + void mark_delayed_add_attribute_aspect_struct(const vespalib::string& name) { _delayed_add_attribute_aspect_struct.insert(name); } + bool is_delayed_add_attribute_aspect_struct(const vespalib::string& name) const noexcept { return _delayed_add_attribute_aspect_struct.find(name) != _delayed_add_attribute_aspect_struct.end(); } + void mark_delayed_remove_attribute_aspect(const vespalib::string& name) { _delayed_remove_attribute_aspect.insert(name); } + bool is_delayed_remove_attribute_aspect(const vespalib::string& name) const noexcept { return _delayed_remove_attribute_aspect.find(name) != _delayed_remove_attribute_aspect.end(); } +public: + AttributeAspectConfigRewriter(const AttributesConfig& old_attributes_config, + const AttributesConfig& new_attributes_config, + const IIndexschemaInspector& old_index_schema_inspector, + const IDocumentTypeInspector& inspector); + ~AttributeAspectConfigRewriter(); + void calculate_delayed_attribute_aspects(); + void build_attributes_config(AttributesConfigBuilder& attributes_config_builder) const; + void build_summary_map_config(const SummarymapConfig& old_summarymap_config, + const SummarymapConfig& new_summarymap_config, + const SummaryConfig& new_summary_config, + SummarymapConfigBuilder& summary_map_config_builder) const; + void build_summary_config(const SummaryConfig& new_summary_config, + SummaryConfigBuilder& summary_config_builder) const; +}; + +AttributeAspectConfigRewriter::AttributeAspectConfigRewriter(const AttributesConfig& old_attributes_config, + const AttributesConfig& new_attributes_config, + const IIndexschemaInspector& old_index_schema_inspector, + const IDocumentTypeInspector& inspector) + : _old_attributes_config(old_attributes_config), + _new_attributes_config(new_attributes_config), + _old_attributes_config_hash(old_attributes_config.attribute), + _new_attributes_config_hash(new_attributes_config.attribute), + _old_index_schema_inspector(old_index_schema_inspector), + _inspector(inspector), + _delayed_add_attribute_aspect(), + _delayed_add_attribute_aspect_struct(), + _delayed_remove_attribute_aspect() { + calculate_delayed_attribute_aspects(); } -std::shared_ptr<AttributeAspectDelayer::AttributesConfig> -AttributeAspectDelayer::getAttributesConfig() const +AttributeAspectConfigRewriter::~AttributeAspectConfigRewriter() = default; + +bool +AttributeAspectConfigRewriter::has_unchanged_field(const vespalib::string& name) const { - return _attributesConfig; + return _inspector.hasUnchangedField(name); } -std::shared_ptr<AttributeAspectDelayer::SummarymapConfig> -AttributeAspectDelayer::getSummarymapConfig() const +bool +AttributeAspectConfigRewriter::should_delay_add_attribute_aspect(const vespalib::string& name) const { - return _summarymapConfig; + if (!has_unchanged_field(name)) { + // No reprocessing due to field type/presence change, just use new config + return false; + } + auto old_attribute_config = _old_attributes_config_hash.lookup(name); + if (old_attribute_config != nullptr) { + return false; // Already added for ready subdb + } + auto new_attribute_config = _new_attributes_config_hash.lookup(name); + if (new_attribute_config == nullptr) { + return false; // Not added for any subdb + } + // Delay addition of attribute aspect since it would trigger reprocessing. + return true; } -namespace { +bool +AttributeAspectConfigRewriter::should_delay_remove_attribute_aspect(const vespalib::string& name) const +{ + if (!has_unchanged_field(name)) { + // No reprocessing due to field type/presence change, just use new config + return false; + } + auto old_attribute_config = _old_attributes_config_hash.lookup(name); + if (old_attribute_config == nullptr) { + return false; // Already removed in all subdbs + } + auto new_attribute_config = _new_attributes_config_hash.lookup(name); + if (new_attribute_config != nullptr) { + return false; // Not removed for ready subdb + } + // Delay removal of attribute aspect if it would trigger reprocessing. + auto old_cfg = ConfigConverter::convert(*old_attribute_config); + return willTriggerReprocessOnAttributeAspectRemoval(old_cfg, _old_index_schema_inspector, name); +} + +bool +AttributeAspectConfigRewriter::calculate_fast_access(const AttributesConfig::Attribute& new_attribute_config) const +{ + auto& name = new_attribute_config.name; + if (!has_unchanged_field(name)) { + // No reprocessing due to field type/presence change, just use new config + return new_attribute_config.fastaccess; + } + auto old_attribute_config = _old_attributes_config_hash.lookup(name); + assert(old_attribute_config != nullptr); + auto old_cfg = ConfigConverter::convert(*old_attribute_config); + if (!old_attribute_config->fastaccess || willTriggerReprocessOnAttributeAspectRemoval(old_cfg, _old_index_schema_inspector, name)) { + // Delay change of fast access flag + return old_attribute_config->fastaccess; + } else { + // Don't delay change of fast access flag from true to + // false when removing attribute aspect in a way that + // doesn't trigger reprocessing. + return new_attribute_config.fastaccess; + } +} void -handleNewAttributes(const AttributesConfig &oldAttributesConfig, - const AttributesConfig &newAttributesConfig, - const SummarymapConfig &newSummarymapConfig, - const IIndexschemaInspector &oldIndexschemaInspector, - const IDocumentTypeInspector &inspector, - AttributesConfigBuilder &attributesConfig, - SummarymapConfigBuilder &summarymapConfig) -{ - vespalib::hash_set<vespalib::string> delayed; - vespalib::hash_set<vespalib::string> delayedStruct; - AttributesConfigHash oldAttrs(oldAttributesConfig.attribute); - for (const auto &newAttr : newAttributesConfig.attribute) { - search::attribute::Config newCfg = ConfigConverter::convert(newAttr); - if (!inspector.hasUnchangedField(newAttr.name)) { - // No reprocessing due to field type change, just use new config - attributesConfig.attribute.emplace_back(newAttr); - } else { - auto oldAttr = oldAttrs.lookup(newAttr.name); - if (oldAttr != nullptr) { - search::attribute::Config oldCfg = ConfigConverter::convert(*oldAttr); - if (willTriggerReprocessOnAttributeAspectRemoval(oldCfg, oldIndexschemaInspector, newAttr.name) || !oldAttr->fastaccess) { - // Delay change of fast access flag - newCfg.setFastAccess(oldAttr->fastaccess); - auto modNewAttr = newAttr; - modNewAttr.fastaccess = oldAttr->fastaccess; - attributesConfig.attribute.emplace_back(modNewAttr); - // TODO: Don't delay change of fast access flag if - // attribute type can change without doucment field - // type changing (needs a smarter attribute - // reprocessing initializer). - } else { - // Don't delay change of fast access flag from true to - // false when removing attribute aspect in a way that - // doesn't trigger reprocessing. - attributesConfig.attribute.emplace_back(newAttr); - } - } else { - // Delay addition of attribute aspect - delayed.insert(newAttr.name); - auto pos = newAttr.name.find('.'); - if (pos != vespalib::string::npos) { - delayedStruct.insert(newAttr.name.substr(0, pos)); - } +AttributeAspectConfigRewriter::calculate_delayed_attribute_aspects() +{ + for (const auto &newAttr : _new_attributes_config.attribute) { + if (should_delay_add_attribute_aspect(newAttr.name)) { + mark_delayed_add_attribute_aspect(newAttr.name); + auto pos = newAttr.name.find('.'); + if (pos != vespalib::string::npos) { + mark_delayed_add_attribute_aspect_struct(newAttr.name.substr(0, pos)); } } } - for (const auto &override : newSummarymapConfig.override) { - if (override.command == "attribute") { - auto itr = delayed.find(source_field(override)); - if (itr == delayed.end()) { - summarymapConfig.override.emplace_back(override); + for (const auto &oldAttr : _old_attributes_config.attribute) { + if (should_delay_remove_attribute_aspect(oldAttr.name)) { + mark_delayed_remove_attribute_aspect(oldAttr.name); + } + } +} + +void +AttributeAspectConfigRewriter::build_attributes_config(AttributesConfigBuilder& attributes_config_builder) const +{ + for (const auto &newAttr : _new_attributes_config.attribute) { + if (is_delayed_add_attribute_aspect(newAttr.name)) { + // Delay addition of attribute aspect + } else { + attributes_config_builder.attribute.emplace_back(newAttr); + attributes_config_builder.attribute.back().fastaccess = calculate_fast_access(newAttr); + } + } + for (const auto &oldAttr : _old_attributes_config.attribute) { + if (is_delayed_remove_attribute_aspect(oldAttr.name)) { + // Delay removal of attribute aspect + attributes_config_builder.attribute.emplace_back(oldAttr); + } + } +} + +void +AttributeAspectConfigRewriter::build_summary_map_config(const SummarymapConfig& old_summarymap_config, + const SummarymapConfig& new_summarymap_config, + const SummaryConfig& new_summary_config, + SummarymapConfigBuilder& summarymap_config_builder) const +{ + KnownSummaryFields knownSummaryFields(new_summary_config); + for (const auto &override : new_summarymap_config.override) { + if (override.command == attribute_dfw_string) { + if (!is_delayed_add_attribute_aspect(source_field(override))) { + summarymap_config_builder.override.emplace_back(override); } - } else if (override.command == "attributecombiner") { - auto itr = delayedStruct.find(source_field(override)); - if (itr == delayedStruct.end()) { - summarymapConfig.override.emplace_back(override); + } else if (override.command == attribute_combiner_dfw_string) { + if (!is_delayed_add_attribute_aspect_struct(source_field(override))) { + summarymap_config_builder.override.emplace_back(override); } - } else if (override.command == "matchedattributeelementsfilter") { - auto itr = delayedStruct.find(source_field(override)); - if (itr == delayedStruct.end()) { - summarymapConfig.override.emplace_back(override); + } else if (override.command == matched_attribute_elements_filter_dfw_string) { + if (!is_delayed_add_attribute_aspect_struct(source_field(override))) { + summarymap_config_builder.override.emplace_back(override); } else { SummarymapConfig::Override mutated_override(override); - mutated_override.command = "matchedelementsfilter"; - summarymapConfig.override.emplace_back(mutated_override); + mutated_override.command = matched_elements_filter_dfw_string; + summarymap_config_builder.override.emplace_back(mutated_override); } } else { - summarymapConfig.override.emplace_back(override); + summarymap_config_builder.override.emplace_back(override); + } + } + for (const auto &override : old_summarymap_config.override) { + if (override.command == attribute_dfw_string) { + if (is_delayed_remove_attribute_aspect(source_field(override)) && knownSummaryFields.known(override.field)) { + summarymap_config_builder.override.emplace_back(override); + } } } } void -handleOldAttributes(const AttributesConfig &oldAttributesConfig, - const AttributesConfig &newAttributesConfig, - const SummarymapConfig &oldSummarymapConfig, - const SummaryConfig &newSummaryConfig, - const IIndexschemaInspector &oldIndexschemaInspector, - const IDocumentTypeInspector &inspector, - AttributesConfigBuilder &attributesConfig, - SummarymapConfigBuilder &summarymapConfig) -{ - vespalib::hash_set<vespalib::string> delayed; - KnownSummaryFields knownSummaryFields(newSummaryConfig); - AttributesConfigHash newAttrs(newAttributesConfig.attribute); - for (const auto &oldAttr : oldAttributesConfig.attribute) { - search::attribute::Config oldCfg = ConfigConverter::convert(oldAttr); - if (inspector.hasUnchangedField(oldAttr.name)) { - auto newAttr = newAttrs.lookup(oldAttr.name); - if (newAttr == nullptr) { - // Delay removal of attribute aspect if it would trigger - // reprocessing. - if (willTriggerReprocessOnAttributeAspectRemoval(oldCfg, oldIndexschemaInspector, oldAttr.name)) { - attributesConfig.attribute.emplace_back(oldAttr); - delayed.insert(oldAttr.name); +AttributeAspectConfigRewriter::build_summary_config(const SummaryConfig& new_summary_config, + SummaryConfigBuilder& summary_config_builder) const +{ + summary_config_builder = new_summary_config; + for (auto &summary_class : summary_config_builder.classes) { + for (auto &summary_field : summary_class.fields) { + if (summary_field.command == attribute_dfw_string) { + if (is_delayed_add_attribute_aspect(source_field(summary_field))) { + remove_docsum_field_rewriter(summary_field); + } + } else if (summary_field.command == attribute_combiner_dfw_string) { + if (is_delayed_add_attribute_aspect_struct(source_field(summary_field))) { + remove_docsum_field_rewriter(summary_field); + } + } else if (summary_field.command == matched_attribute_elements_filter_dfw_string) { + if (is_delayed_add_attribute_aspect_struct(source_field(summary_field)) || + is_delayed_add_attribute_aspect(source_field(summary_field))) { + summary_field.command = matched_elements_filter_dfw_string; + } + } else if (summary_field.command == matched_elements_filter_dfw_string) { + if (is_delayed_remove_attribute_aspect(source_field(summary_field))) { + summary_field.command = matched_attribute_elements_filter_dfw_string; + } + } else if (summary_field.command == "") { + if (is_delayed_remove_attribute_aspect(summary_field.name)) { + summary_field.command = attribute_dfw_string; + summary_field.source = summary_field.name; + } + } else if (summary_field.command == copy_dfw_string) { + if (is_delayed_remove_attribute_aspect(source_field(summary_field))) { + summary_field.command = attribute_dfw_string; + summary_field.source = source_field(summary_field); } } } } - for (const auto &override : oldSummarymapConfig.override) { - if (override.command == "attribute") { - auto itr = delayed.find(source_field(override)); - if (itr != delayed.end() && knownSummaryFields.known(override.field)) { - summarymapConfig.override.emplace_back(override); - } - } - } } } +AttributeAspectDelayer::AttributeAspectDelayer() + : _attributesConfig(std::make_shared<AttributesConfigBuilder>()), + _summarymapConfig(std::make_shared<SummarymapConfigBuilder>()), + _summaryConfig(std::make_shared<SummaryConfigBuilder>()) +{ +} + +AttributeAspectDelayer::~AttributeAspectDelayer() +{ +} + +std::shared_ptr<AttributeAspectDelayer::AttributesConfig> +AttributeAspectDelayer::getAttributesConfig() const +{ + return _attributesConfig; +} + +std::shared_ptr<AttributeAspectDelayer::SummarymapConfig> +AttributeAspectDelayer::getSummarymapConfig() const +{ + return _summarymapConfig; +} + +std::shared_ptr<AttributeAspectDelayer::SummaryConfig> +AttributeAspectDelayer::getSummaryConfig() const +{ + return _summaryConfig; +} + void AttributeAspectDelayer::setup(const AttributesConfig &oldAttributesConfig, const SummarymapConfig &oldSummarymapConfig, @@ -215,14 +374,17 @@ AttributeAspectDelayer::setup(const AttributesConfig &oldAttributesConfig, const IIndexschemaInspector &oldIndexschemaInspector, const IDocumentTypeInspector &inspector) { - handleNewAttributes(oldAttributesConfig, newAttributesConfig, - newSummarymapConfig, - oldIndexschemaInspector, inspector, - *_attributesConfig, *_summarymapConfig); - handleOldAttributes(oldAttributesConfig, newAttributesConfig, - oldSummarymapConfig, newSummaryConfig, - oldIndexschemaInspector, inspector, - *_attributesConfig, *_summarymapConfig); + AttributeAspectConfigRewriter cfg_rewriter(oldAttributesConfig, + newAttributesConfig, + oldIndexschemaInspector, + inspector); + cfg_rewriter.build_attributes_config(*_attributesConfig); + cfg_rewriter.build_summary_map_config(oldSummarymapConfig, + newSummarymapConfig, + newSummaryConfig, + *_summarymapConfig); + cfg_rewriter.build_summary_config(newSummaryConfig, + *_summaryConfig); } } // namespace proton diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.h b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.h index 9ffb23dca8d..9d8338349d1 100644 --- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.h +++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.h @@ -29,10 +29,12 @@ class AttributeAspectDelayer using IndexschemaConfig = const vespa::config::search::internal::InternalIndexschemaType; using SummarymapConfigBuilder = vespa::config::search::internal::InternalSummarymapType; using SummarymapConfig = const vespa::config::search::internal::InternalSummarymapType; + using SummaryConfigBuilder = vespa::config::search::internal::InternalSummaryType; using SummaryConfig = const vespa::config::search::internal::InternalSummaryType; std::shared_ptr<AttributesConfigBuilder> _attributesConfig; std::shared_ptr<SummarymapConfigBuilder> _summarymapConfig; + std::shared_ptr<SummaryConfigBuilder> _summaryConfig; public: AttributeAspectDelayer(); @@ -52,6 +54,7 @@ public: std::shared_ptr<AttributesConfig> getAttributesConfig() const; std::shared_ptr<SummarymapConfig> getSummarymapConfig() const; + std::shared_ptr<SummaryConfig> getSummaryConfig() const; }; } // namespace proton diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp b/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp index 942dac63955..9a4ec121d3b 100644 --- a/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp +++ b/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp @@ -71,13 +71,13 @@ makeSlimeParams(size_t chunkSize) { vespalib::Slime::UP DocsumContext::createSlimeReply() { - _docsumWriter.InitState(_attrMgr, &_docsumState); + IDocsumWriter::ResolveClassInfo rci = _docsumWriter.resolveClassInfo(_docsumState._args.getResultClassName()); + _docsumWriter.InitState(_attrMgr, _docsumState, rci); const size_t estimatedChunkSize(std::min(0x200000ul, _docsumState._docsumbuf.size()*0x400ul)); vespalib::Slime::UP response(std::make_unique<vespalib::Slime>(makeSlimeParams(estimatedChunkSize))); Cursor & root = response->setObject(); Cursor & array = root.setArray(DOCSUMS); const Symbol docsumSym = response->insert(DOCSUM); - IDocsumWriter::ResolveClassInfo rci = _docsumWriter.resolveClassInfo(_docsumState._args.getResultClassName()); _docsumState._omit_summary_features = (rci.outputClass != nullptr) ? rci.outputClass->omit_summary_features() : true; uint32_t num_ok(0); for (uint32_t docId : _docsumState._docsumbuf) { diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp index 3e3a3529e46..6a0133e913d 100644 --- a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp +++ b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp @@ -9,7 +9,7 @@ #include <vespa/juniper/rpinterface.h> #include <vespa/searchcore/proton/flushengine/shrink_lid_space_flush_target.h> #include <vespa/vespalib/util/lambdatask.h> -#include <vespa/searchsummary/docsummary/docsumconfig.h> +#include <vespa/searchsummary/docsummary/docsum_field_writer_factory.h> #include <vespa/searchsummary/docsummary/keywordextractor.h> #include <vespa/vespalib/util/exceptions.h> #include <vespa/fastlib/text/normwordfolder.h> @@ -92,8 +92,10 @@ SummarySetup(const vespalib::string & baseDir, const SummaryConfig & summaryCfg, _docStore(std::move(docStore)), _repo(std::move(repo)) { + _juniperConfig = std::make_unique<juniper::Juniper>(&_juniperProps, _wordFolder.get()); auto resultConfig = std::make_unique<ResultConfig>(); - if (!resultConfig->ReadConfig(summaryCfg, make_string("SummaryManager(%s)", baseDir.c_str()).c_str())) { + auto docsum_field_writer_factory = std::make_unique<DocsumFieldWriterFactory>(summaryCfg.usev8geopositions, *this); + if (!resultConfig->ReadConfig(summaryCfg, make_string("SummaryManager(%s)", baseDir.c_str()).c_str(), *docsum_field_writer_factory)) { std::ostringstream oss; ::config::OstreamConfigWriter writer(oss); writer.write(summaryCfg); @@ -101,11 +103,10 @@ SummarySetup(const vespalib::string & baseDir, const SummaryConfig & summaryCfg, (make_string("Could not initialize summary result config for directory '%s' based on summary config '%s'", baseDir.c_str(), oss.str().c_str())); } + docsum_field_writer_factory.reset(); - _juniperConfig = std::make_unique<juniper::Juniper>(&_juniperProps, _wordFolder.get()); _docsumWriter = std::make_unique<DynamicDocsumWriter>(std::move(resultConfig), std::unique_ptr<KeywordExtractor>()); - DynamicDocsumConfig dynCfg(*this, _docsumWriter.get()); - dynCfg.configure(summarymapCfg); + (void) summarymapCfg; } IDocsumStore::UP diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp index d99bdc97c90..b93f96aa3bb 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp @@ -321,8 +321,10 @@ DocumentDBConfig::makeDelayedAttributeAspectConfig(const SP &newCfg, const Docum attributeAspectDelayer.setup(oldCfg.getAttributesConfig(), oldCfg.getSummarymapConfig(), n.getAttributesConfig(), n.getSummaryConfig(), n.getSummarymapConfig(), oldIndexschemaInspector, inspector); - bool delayedAttributeAspects = (n.getAttributesConfig() != *attributeAspectDelayer.getAttributesConfig()) || - (n.getSummarymapConfig() != *attributeAspectDelayer.getSummarymapConfig()); + bool attributes_config_changed = (n.getAttributesConfig() != *attributeAspectDelayer.getAttributesConfig()); + bool summarymap_config_changed = (n.getSummarymapConfig() != *attributeAspectDelayer.getSummarymapConfig()); + bool summary_config_changed = (n.getSummaryConfig() != *attributeAspectDelayer.getSummaryConfig()); + bool delayedAttributeAspects = (attributes_config_changed || summarymap_config_changed || summary_config_changed); if (!delayedAttributeAspects) { return newCfg; } @@ -333,9 +335,9 @@ DocumentDBConfig::makeDelayedAttributeAspectConfig(const SP &newCfg, const Docum n._rankingExpressions, n._onnxModels, n._indexschema, - attributeAspectDelayer.getAttributesConfig(), - n._summary, - attributeAspectDelayer.getSummarymapConfig(), + (attributes_config_changed ? attributeAspectDelayer.getAttributesConfig() : n._attributes), + (summary_config_changed ? attributeAspectDelayer.getSummaryConfig() : n._summary), + (summarymap_config_changed ? attributeAspectDelayer.getSummarymapConfig() : n._summarymap), n._juniperrc, n._documenttypes, n._repo, diff --git a/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt b/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt index 963c94f7796..8070bed8b03 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt +++ b/searchsummary/src/vespa/searchsummary/docsummary/CMakeLists.txt @@ -7,7 +7,6 @@ vespa_add_library(searchsummary_docsummary OBJECT attributedfw.cpp check_undefined_value_visitor.cpp copy_dfw.cpp - docsumconfig.cpp docsum_field_writer.cpp docsum_field_writer_factory.cpp docsum_store_document.cpp diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumconfig.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsumconfig.cpp deleted file mode 100644 index 0c2adcbfaa5..00000000000 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumconfig.cpp +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include "docsumconfig.h" -#include "docsum_field_writer_factory.h" -#include "docsumwriter.h" -#include <vespa/vespalib/util/exceptions.h> - -namespace search::docsummary { - -using vespalib::IllegalArgumentException; -using vespalib::make_string; - -const ResultConfig & -DynamicDocsumConfig::getResultConfig() const { - return *_writer->GetResultConfig(); -} - -std::unique_ptr<IDocsumFieldWriterFactory> -DynamicDocsumConfig::make_docsum_field_writer_factory() -{ - return std::make_unique<DocsumFieldWriterFactory>(getResultConfig().useV8geoPositions(), getEnvironment()); -} - -void -DynamicDocsumConfig::configure(const vespa::config::search::SummarymapConfig &cfg) -{ - std::vector<string> strCfg; - auto docsum_field_writer_factory = make_docsum_field_writer_factory(); - for (const auto & o : cfg.override) { - bool rc(false); - auto fieldWriter = docsum_field_writer_factory->create_docsum_field_writer(o.field, o.command, o.arguments, rc); - if (rc && fieldWriter) { - rc = _writer->Override(o.field.c_str(), std::move(fieldWriter)); // OBJECT HAND-OVER - } - if (!rc) { - throw IllegalArgumentException(o.command + " override operation failed during initialization"); - } - } -} - -} diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumconfig.h b/searchsummary/src/vespa/searchsummary/docsummary/docsumconfig.h deleted file mode 100644 index 2b0f90a8e8b..00000000000 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumconfig.h +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#pragma once - -#include <vespa/config-summarymap.h> - -namespace search { class MatchingElementsFields; } -namespace search::docsummary { - -class IDocsumEnvironment; -class IDocsumFieldWriterFactory; -class DocsumFieldWriter; -class DynamicDocsumWriter; -class ResultConfig; - -class DynamicDocsumConfig -{ -public: - DynamicDocsumConfig(const IDocsumEnvironment& env, DynamicDocsumWriter * writer) : - _env(env), - _writer(writer) - { } - virtual ~DynamicDocsumConfig() = default; - void configure(const vespa::config::search::SummarymapConfig &cfg); -protected: - using string = vespalib::string; - const IDocsumEnvironment& getEnvironment() { return _env; } - const ResultConfig & getResultConfig() const; - - virtual std::unique_ptr<IDocsumFieldWriterFactory> make_docsum_field_writer_factory(); -private: - const IDocsumEnvironment& _env; - DynamicDocsumWriter * _writer; -}; - -} - diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.cpp index 89de20b6a81..181af0ca9a3 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.cpp @@ -60,7 +60,7 @@ DynamicDocsumWriter::insertDocsum(const ResolveClassInfo & rci, uint32_t docid, vespalib::slime::Cursor & docsum = topInserter.insertObject(); for (uint32_t i = 0; i < rci.outputClass->GetNumEntries(); ++i) { const ResConfigEntry *resCfg = rci.outputClass->GetEntry(i); - const DocsumFieldWriter *writer = _overrideTable[resCfg->_enumValue].get(); + const DocsumFieldWriter *writer = resCfg->_docsum_field_writer.get(); if (state->_args.needField(resCfg->_bindname) && ! writer->isDefaultValue(docid, state)) { const Memory field_name(resCfg->_bindname.data(), resCfg->_bindname.size()); ObjectInserter inserter(docsum, field_name); @@ -78,7 +78,7 @@ DynamicDocsumWriter::insertDocsum(const ResolveClassInfo & rci, uint32_t docid, for (uint32_t i = 0; i < rci.outputClass->GetNumEntries(); ++i) { const ResConfigEntry *outCfg = rci.outputClass->GetEntry(i); if ( ! state->_args.needField(outCfg->_bindname)) continue; - const DocsumFieldWriter *writer = _overrideTable[outCfg->_enumValue].get(); + const DocsumFieldWriter *writer = outCfg->_docsum_field_writer.get(); const Memory field_name(outCfg->_bindname.data(), outCfg->_bindname.size()); ObjectInserter inserter(docsum, field_name); if (writer != nullptr) { @@ -96,60 +96,31 @@ DynamicDocsumWriter::insertDocsum(const ResolveClassInfo & rci, uint32_t docid, DynamicDocsumWriter::DynamicDocsumWriter(std::unique_ptr<ResultConfig> config, std::unique_ptr<KeywordExtractor> extractor) : _resultConfig(std::move(config)), - _keywordExtractor(std::move(extractor)), - _numFieldWriterStates(0), - _overrideTable(_resultConfig->GetFieldNameEnum().GetNumEntries()) + _keywordExtractor(std::move(extractor)) { } DynamicDocsumWriter::~DynamicDocsumWriter() = default; -bool -DynamicDocsumWriter::Override(const char *fieldName, std::unique_ptr<DocsumFieldWriter> writer) -{ - uint32_t fieldEnumValue = _resultConfig->GetFieldNameEnum().Lookup(fieldName); - - if (fieldEnumValue >= _overrideTable.size() || _overrideTable[fieldEnumValue]) { - if (fieldEnumValue >= _overrideTable.size()) { - LOG(warning, "cannot override docsum field '%s'; undefined field name", fieldName); - } else if (_overrideTable[fieldEnumValue] != nullptr) { - LOG(warning, "cannot override docsum field '%s'; already overridden", fieldName); - } - return false; - } - - writer->setIndex(fieldEnumValue); - auto writer_ptr = writer.get(); - _overrideTable[fieldEnumValue] = std::move(writer); - if (writer_ptr->setFieldWriterStateIndex(_numFieldWriterStates)) { - ++_numFieldWriterStates; - } - - bool generated = writer_ptr->IsGenerated(); - for (auto & result_class : *_resultConfig) { - if (result_class.GetIndexFromEnumValue(fieldEnumValue) >= 0) { - result_class.getDynamicInfo().update_override_counts(generated); - } - } - - return true; -} - - void -DynamicDocsumWriter::InitState(const IAttributeManager & attrMan, GetDocsumsState *state) +DynamicDocsumWriter::InitState(const IAttributeManager & attrMan, GetDocsumsState& state, const ResolveClassInfo& rci) { - state->_kwExtractor = _keywordExtractor.get(); - state->_attrCtx = attrMan.createContext(); - state->_attributes.resize(_overrideTable.size()); - state->_fieldWriterStates.resize(_numFieldWriterStates); - for (size_t i(0); i < state->_attributes.size(); i++) { - const DocsumFieldWriter *fw = _overrideTable[i].get(); + state._kwExtractor = _keywordExtractor.get(); + state._attrCtx = attrMan.createContext(); + auto result_class = rci.outputClass; + if (result_class == nullptr) { + return; + } + size_t num_entries = result_class->GetNumEntries(); + state._attributes.resize(num_entries); + state._fieldWriterStates.resize(result_class->get_num_field_writer_states()); + for (size_t i(0); i < num_entries; i++) { + const DocsumFieldWriter *fw = result_class->GetEntry(i)->_docsum_field_writer.get(); if (fw) { const vespalib::string & attributeName = fw->getAttributeName(); if (!attributeName.empty()) { - state->_attributes[i] = state->_attrCtx->getAttribute(attributeName); + state._attributes[i] = state._attrCtx->getAttribute(attributeName); } } } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.h b/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.h index 9ae44623475..c0579638593 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/docsumwriter.h @@ -36,7 +36,7 @@ public: }; virtual ~IDocsumWriter() = default; - virtual void InitState(const search::IAttributeManager & attrMan, GetDocsumsState *state) = 0; + virtual void InitState(const search::IAttributeManager & attrMan, GetDocsumsState& state, const ResolveClassInfo& rci) = 0; virtual void insertDocsum(const ResolveClassInfo & rci, uint32_t docid, GetDocsumsState *state, IDocsumStore *docinfos, Inserter & target) = 0; virtual ResolveClassInfo resolveClassInfo(vespalib::stringref outputClassName) const = 0; @@ -49,8 +49,6 @@ class DynamicDocsumWriter : public IDocsumWriter private: std::unique_ptr<ResultConfig> _resultConfig; std::unique_ptr<KeywordExtractor> _keywordExtractor; - uint32_t _numFieldWriterStates; - std::vector<std::unique_ptr<const DocsumFieldWriter>> _overrideTable; ResolveClassInfo resolveOutputClass(vespalib::stringref outputClassName) const; @@ -62,8 +60,7 @@ public: const ResultConfig *GetResultConfig() { return _resultConfig.get(); } - bool Override(const char *fieldName, std::unique_ptr<DocsumFieldWriter> writer); - void InitState(const search::IAttributeManager & attrMan, GetDocsumsState *state) override; + void InitState(const search::IAttributeManager & attrMan, GetDocsumsState& state, const ResolveClassInfo& rci) override; void insertDocsum(const ResolveClassInfo & outputClassInfo, uint32_t docid, GetDocsumsState *state, IDocsumStore *docinfos, Inserter & inserter) override; diff --git a/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.cpp b/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.cpp index 668fb9b519b..b00b5d975a2 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.cpp @@ -1,16 +1,20 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "res_config_entry.h" +#include "docsum_field_writer.h" namespace search::docsummary { ResConfigEntry::ResConfigEntry() noexcept : _type(RES_BAD), _bindname(), - _enumValue(0) + _enumValue(0), + _docsum_field_writer() { } ResConfigEntry::~ResConfigEntry() = default; +ResConfigEntry::ResConfigEntry(ResConfigEntry&&) noexcept = default; + } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.h b/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.h index 771125b1f45..6277e955c3d 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/res_config_entry.h @@ -4,9 +4,12 @@ #include "res_type.h" #include <vespa/vespalib/stllike/string.h> +#include <memory> namespace search::docsummary { +class DocsumFieldWriter; + /** * This struct describes a single docsum field (name and type). **/ @@ -14,8 +17,10 @@ struct ResConfigEntry { ResType _type; vespalib::string _bindname; int _enumValue; + std::unique_ptr<DocsumFieldWriter> _docsum_field_writer; ResConfigEntry() noexcept; ~ResConfigEntry(); + ResConfigEntry(ResConfigEntry&&) noexcept; }; } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp index 9930c39cb7b..78a9804ccd4 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "resultclass.h" +#include "docsum_field_writer.h" #include "resultconfig.h" #include <vespa/vespalib/stllike/hashtable.hpp> #include <cassert> @@ -14,7 +15,8 @@ ResultClass::ResultClass(const char *name, util::StringEnum & fieldEnum) _fieldEnum(fieldEnum), _enumMap(), _dynInfo(), - _omit_summary_features(false) + _omit_summary_features(false), + _num_field_writer_states(0) { } @@ -28,7 +30,7 @@ ResultClass::GetIndexFromName(const char* name) const } bool -ResultClass::AddConfigEntry(const char *name, ResType type) +ResultClass::AddConfigEntry(const char *name, ResType type, std::unique_ptr<DocsumFieldWriter> docsum_field_writer) { if (_nameMap.find(name) != _nameMap.end()) return false; @@ -39,10 +41,25 @@ ResultClass::AddConfigEntry(const char *name, ResType type) e._bindname = name; e._enumValue = _fieldEnum.Add(name); assert(e._enumValue >= 0); - _entries.push_back(e); + if (docsum_field_writer) { + docsum_field_writer->setIndex(_entries.size()); + bool generated = docsum_field_writer->IsGenerated(); + getDynamicInfo().update_override_counts(generated); + if (docsum_field_writer->setFieldWriterStateIndex(_num_field_writer_states)) { + ++_num_field_writer_states; + } + } + e._docsum_field_writer = std::move(docsum_field_writer); + _entries.push_back(std::move(e)); return true; } +bool +ResultClass::AddConfigEntry(const char *name, ResType type) +{ + return AddConfigEntry(name, type, {}); +} + void ResultClass::CreateEnumMap() { diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h index 39e94e5050e..e11032d1aac 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h @@ -50,6 +50,7 @@ private: // Whether or not summary features should be omitted when filling this summary class. // As default, summary features are always included. bool _omit_summary_features; + size_t _num_field_writer_states; public: typedef std::unique_ptr<ResultClass> UP; @@ -95,10 +96,13 @@ public: * @return true(success)/false(fail) * @param name the name of the field to add. * @param type the type of the field to add. + * @param docsum_field_writer field writer for writing field **/ + bool AddConfigEntry(const char *name, ResType type, std::unique_ptr<DocsumFieldWriter> docsum_field_writer); bool AddConfigEntry(const char *name, ResType type); + /** * This method may be called to create an internal mapping from * field name enumerated value to field index. When building up a @@ -169,6 +173,8 @@ public: bool omit_summary_features() const { return _omit_summary_features; } + + size_t get_num_field_writer_states() const noexcept { return _num_field_writer_states; } }; } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp b/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp index f5e4c5d34cf..63ccf80d4e7 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp @@ -1,6 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "resultconfig.h" +#include "docsum_field_writer.h" +#include "docsum_field_writer_factory.h" #include "resultclass.h" #include <vespa/vespalib/util/exceptions.h> #include <vespa/vespalib/stllike/hash_map.hpp> @@ -99,7 +101,7 @@ bool ResultConfig::wantedV8geoPositions() { } bool -ResultConfig::ReadConfig(const vespa::config::search::SummaryConfig &cfg, const char *configId) +ResultConfig::ReadConfig(const vespa::config::search::SummaryConfig &cfg, const char *configId, IDocsumFieldWriterFactory& docsum_field_writer_factory) { bool rc = true; Reset(); @@ -129,10 +131,20 @@ ResultConfig::ReadConfig(const vespa::config::search::SummaryConfig &cfg, const for (unsigned int j = 0; rc && (j < cfg_class.fields.size()); j++) { const char *fieldtype = cfg_class.fields[j].type.c_str(); const char *fieldname = cfg_class.fields[j].name.c_str(); + vespalib::string override_name = cfg_class.fields[j].command; + vespalib::string source_name = cfg_class.fields[j].source; auto res_type = ResTypeUtils::get_res_type(fieldtype); LOG(debug, "Reconfiguring class '%s' field '%s' of type '%s'", cfg_class.name.c_str(), fieldname, fieldtype); if (res_type != RES_BAD) { - rc = resClass->AddConfigEntry(fieldname, res_type); + std::unique_ptr<DocsumFieldWriter> docsum_field_writer; + if (!override_name.empty()) { + docsum_field_writer = docsum_field_writer_factory.create_docsum_field_writer(fieldname, override_name, source_name, rc); + if (!rc) { + LOG(error, "%s override operation failed during initialization", override_name.c_str()); + break; + } + } + rc = resClass->AddConfigEntry(fieldname, res_type, std::move(docsum_field_writer)); } else { LOG(error, "%s %s.fields[%d]: unknown type '%s'", configId, cfg_class.name.c_str(), j, fieldtype); rc = false; diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.h b/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.h index 220c988b634..9aaf6fa65b3 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.h @@ -8,6 +8,7 @@ namespace search::docsummary { +class IDocsumFieldWriterFactory; class ResultClass; /** @@ -176,7 +177,7 @@ public: * @return true(success)/false(fail) * @param configId reference on server **/ - bool ReadConfig(const vespa::config::search::SummaryConfig &cfg, const char *configId); + bool ReadConfig(const vespa::config::search::SummaryConfig &cfg, const char *configId, IDocsumFieldWriterFactory& docsum_field_writer_factory); }; } diff --git a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp index 9c39d929e4c..0bf41f9a379 100644 --- a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp @@ -161,7 +161,7 @@ SearchVisitor::SummaryGenerator::get_streaming_docsums_state(const vespalib::str if (_stack_dump.has_value()) { ds._args.SetStackDump(_stack_dump.value().size(), _stack_dump.value().data()); } - _docsumWriter->InitState(_attr_manager, &ds); + _docsumWriter->InitState(_attr_manager, ds, state->get_resolve_class_info()); auto insres = _docsum_states.insert(std::make_pair(summary_class, std::move(state))); return *insres.first->second; } diff --git a/streamingvisitors/src/vespa/vsm/vsm/CMakeLists.txt b/streamingvisitors/src/vespa/vsm/vsm/CMakeLists.txt index 6afb61078c7..1e766baf0ed 100644 --- a/streamingvisitors/src/vespa/vsm/vsm/CMakeLists.txt +++ b/streamingvisitors/src/vespa/vsm/vsm/CMakeLists.txt @@ -9,7 +9,6 @@ vespa_add_library(vsm_vsmbase OBJECT slimefieldwriter.cpp snippetmodifier.cpp vsm-adapter.cpp - docsumconfig.cpp DEPENDS vsm_vconfig ) diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsumconfig.cpp b/streamingvisitors/src/vespa/vsm/vsm/docsumconfig.cpp deleted file mode 100644 index 59e56e657be..00000000000 --- a/streamingvisitors/src/vespa/vsm/vsm/docsumconfig.cpp +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include "docsumconfig.h" -#include <vespa/searchsummary/docsummary/resultconfig.h> -#include <vespa/vsm/config/config-vsmfields.h> -#include "docsum_field_writer_factory.h" - -using vespa::config::search::vsm::VsmfieldsConfig; - -namespace vsm { - -DynamicDocsumConfig::DynamicDocsumConfig(const search::docsummary::IDocsumEnvironment& env, search::docsummary::DynamicDocsumWriter* writer, std::shared_ptr<VsmfieldsConfig> vsm_fields_config) - : Parent(env, writer), - _vsm_fields_config(std::move(vsm_fields_config)) -{ -} - -std::unique_ptr<search::docsummary::IDocsumFieldWriterFactory> -DynamicDocsumConfig::make_docsum_field_writer_factory() -{ - return std::make_unique<DocsumFieldWriterFactory>(getResultConfig().useV8geoPositions(), getEnvironment(), *_vsm_fields_config); -} - -} diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsumconfig.h b/streamingvisitors/src/vespa/vsm/vsm/docsumconfig.h deleted file mode 100644 index 760d493dbc1..00000000000 --- a/streamingvisitors/src/vespa/vsm/vsm/docsumconfig.h +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#pragma once - -#include <vespa/searchsummary/docsummary/docsumconfig.h> - -namespace vespa::config::search::vsm { -namespace internal { class InternalVsmfieldsType; } -typedef const internal::InternalVsmfieldsType VsmfieldsConfig; -} -namespace vsm { - -class DynamicDocsumConfig : public search::docsummary::DynamicDocsumConfig -{ -public: - using Parent = search::docsummary::DynamicDocsumConfig; - using VsmfieldsConfig = vespa::config::search::vsm::VsmfieldsConfig; -private: - std::shared_ptr<VsmfieldsConfig> _vsm_fields_config; -public: - DynamicDocsumConfig(const search::docsummary::IDocsumEnvironment& env, search::docsummary::DynamicDocsumWriter* writer, std::shared_ptr<VsmfieldsConfig> vsm_fields_config); -private: - std::unique_ptr<search::docsummary::IDocsumFieldWriterFactory> make_docsum_field_writer_factory() override; -}; - -} - diff --git a/streamingvisitors/src/vespa/vsm/vsm/vsm-adapter.cpp b/streamingvisitors/src/vespa/vsm/vsm/vsm-adapter.cpp index 2bf6b2f3972..e220cf685e2 100644 --- a/streamingvisitors/src/vespa/vsm/vsm/vsm-adapter.cpp +++ b/streamingvisitors/src/vespa/vsm/vsm/vsm-adapter.cpp @@ -1,7 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "vsm-adapter.hpp" -#include "docsumconfig.h" +#include "docsum_field_writer_factory.h" #include "i_matching_elements_filler.h" #include <vespa/searchlib/common/matching_elements.h> #include <vespa/searchsummary/docsummary/keywordextractor.h> @@ -144,16 +144,18 @@ VSMAdapter::configure(const VSMConfigSnapshot & snapshot) // create new docsum tools auto docsumTools = std::make_unique<DocsumTools>(); - // configure juniper (used when configuring DynamicDocsumConfig) + // configure juniper (used by search::docsummary::DocsumFieldWriterFactory) _juniperProps = std::make_unique<JuniperProperties>(*juniperrc); auto juniper = std::make_unique<juniper::Juniper>(_juniperProps.get(), &_wordFolder); docsumTools->setJuniper(std::move(juniper)); // init result config auto resCfg = std::make_unique<ResultConfig>(); - if ( ! resCfg->ReadConfig(*summary.get(), _configId.c_str())) { + auto docsum_field_writer_factory = std::make_unique<DocsumFieldWriterFactory>(summary.get()->usev8geopositions, *docsumTools, *_fieldsCfg.get()); + if ( ! resCfg->ReadConfig(*summary.get(), _configId.c_str(), *docsum_field_writer_factory)) { throw std::runtime_error("(re-)configuration of VSM (docsum tools) failed due to bad summary config"); } + docsum_field_writer_factory.reset(); // init keyword extractor auto kwExtractor = std::make_unique<KeywordExtractor>(nullptr); @@ -165,10 +167,6 @@ VSMAdapter::configure(const VSMConfigSnapshot & snapshot) auto writer = std::make_unique<DynamicDocsumWriter>(std::move(resCfg), std::move(kwExtractor)); docsumTools->set_writer(std::move(writer)); - // configure dynamic docsum writer - DynamicDocsumConfig dynDocsumConfig(*docsumTools, docsumTools->getDocsumWriter(), _fieldsCfg.get()); - dynDocsumConfig.configure(*summaryMap.get()); - // configure new docsum tools if (docsumTools->obtainFieldNames(vsmSummary)) { // latch new docsum tools into production |