diff options
36 files changed, 558 insertions, 189 deletions
diff --git a/container-search/abi-spec.json b/container-search/abi-spec.json index fbcf84f5b5d..84411b31274 100644 --- a/container-search/abi-spec.json +++ b/container-search/abi-spec.json @@ -209,7 +209,6 @@ ], "methods" : [ "public void <init>(byte[])", - "public void <init>(byte[], boolean)", "public byte[] value()", "public int compareTo(com.yahoo.prelude.hitfield.RawBase64)", "public java.lang.String toString()", diff --git a/container-search/src/main/java/com/yahoo/prelude/hitfield/RawBase64.java b/container-search/src/main/java/com/yahoo/prelude/hitfield/RawBase64.java index 71964054e0d..485e2c9a8c3 100644 --- a/container-search/src/main/java/com/yahoo/prelude/hitfield/RawBase64.java +++ b/container-search/src/main/java/com/yahoo/prelude/hitfield/RawBase64.java @@ -10,15 +10,11 @@ import java.util.Objects; * @author baldersheim */ public class RawBase64 implements Comparable<RawBase64> { + private final static Base64.Encoder encoder = Base64.getEncoder().withoutPadding(); private final byte[] content; - private final boolean withoutPadding; public RawBase64(byte[] content) { - this(content, false); - } - public RawBase64(byte[] content, boolean withoutPadding) { Objects.requireNonNull(content); this.content = content; - this.withoutPadding = withoutPadding; } public byte [] value() { return content; } @@ -30,9 +26,7 @@ public class RawBase64 implements Comparable<RawBase64> { @Override public String toString() { - return withoutPadding - ? Base64.getEncoder().withoutPadding().encodeToString(content) - : Base64.getEncoder().encodeToString(content); + return encoder.encodeToString(content); } @Override diff --git a/container-search/src/main/java/com/yahoo/search/grouping/result/RawBucketId.java b/container-search/src/main/java/com/yahoo/search/grouping/result/RawBucketId.java index dc8a2efb5cb..9b5ad6660b0 100644 --- a/container-search/src/main/java/com/yahoo/search/grouping/result/RawBucketId.java +++ b/container-search/src/main/java/com/yahoo/search/grouping/result/RawBucketId.java @@ -18,8 +18,6 @@ public class RawBucketId extends BucketGroupId<RawBase64> { * @param to The identifying exclusive-to raw buffer. */ public RawBucketId(byte[] from, byte[] to) { - super("raw_bucket", - new RawBase64(from, true), - new RawBase64(to, true)); + super("raw_bucket", new RawBase64(from), new RawBase64(to)); } } diff --git a/container-search/src/main/java/com/yahoo/search/grouping/result/RawId.java b/container-search/src/main/java/com/yahoo/search/grouping/result/RawId.java index f0ae9628c22..fd0d38c37fd 100644 --- a/container-search/src/main/java/com/yahoo/search/grouping/result/RawId.java +++ b/container-search/src/main/java/com/yahoo/search/grouping/result/RawId.java @@ -16,6 +16,6 @@ public class RawId extends ValueGroupId<RawBase64> { * @param value The identifying byte array. */ public RawId(byte[] value) { - super("raw", new RawBase64(value, true)); + super("raw", new RawBase64(value)); } } diff --git a/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java b/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java index 2333a180690..e746706f9c5 100644 --- a/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java +++ b/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java @@ -245,7 +245,7 @@ class ResultBuilder { private Object convertResult(Object value) { if (value instanceof RawData raw) { - return new RawBase64(raw.getData(), true); + return new RawBase64(raw.getData()); } return value; } diff --git a/container-search/src/test/java/com/yahoo/search/grouping/result/GroupIdTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/result/GroupIdTestCase.java index bdc52685140..77ed858b14b 100644 --- a/container-search/src/test/java/com/yahoo/search/grouping/result/GroupIdTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/grouping/result/GroupIdTestCase.java @@ -26,10 +26,10 @@ public class GroupIdTestCase { assertEquals(9L, rangeId.getTo()); valueId = new RawId(new byte[]{6, 9}); - assertEquals(new RawBase64(new byte[]{6, 9}, true), valueId.getValue()); + assertEquals(new RawBase64(new byte[]{6, 9}), valueId.getValue()); rangeId = new RawBucketId(new byte[]{6, 9}, new byte[]{9, 6}); - assertEquals(new RawBase64(new byte[]{6, 9}, true), rangeId.getFrom()); - assertEquals(new RawBase64(new byte[]{9, 6}, true), rangeId.getTo()); + assertEquals(new RawBase64(new byte[]{6, 9}), rangeId.getFrom()); + assertEquals(new RawBase64(new byte[]{9, 6}), rangeId.getTo()); valueId = new StringId("69"); assertEquals("69", valueId.getValue()); diff --git a/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java b/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java index 110564bea46..795f8e93187 100644 --- a/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java +++ b/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java @@ -5,7 +5,6 @@ import com.fasterxml.jackson.core.JsonGenerator; import com.yahoo.document.DataType; import com.yahoo.document.DocumentId; import com.yahoo.document.Field; -import com.yahoo.document.PositionDataType; import com.yahoo.document.PrimitiveDataType; import com.yahoo.document.datatypes.Array; import com.yahoo.document.datatypes.BoolFieldValue; @@ -41,7 +40,6 @@ import java.nio.charset.StandardCharsets; import java.util.Base64; import java.util.Iterator; import java.util.Map; -import java.util.Set; /** * @author Steinar Knutsen @@ -49,7 +47,7 @@ import java.util.Set; */ public class JsonSerializationHelper { - private final static Base64.Encoder base64Encoder = Base64.getEncoder(); // Important: _basic_ format + private final static Base64.Encoder base64Encoder = Base64.getEncoder().withoutPadding(); // Important: _basic_ format static class JsonSerializationException extends RuntimeException { public JsonSerializationException(Exception base) { @@ -166,8 +164,7 @@ public class JsonSerializationHelper { public static void serializeStructField(FieldWriter fieldWriter, JsonGenerator generator, FieldBase field, Struct value) { DataType dt = value.getDataType(); - if (dt instanceof GeoPosType) { - var gpt = (GeoPosType)dt; + if (dt instanceof GeoPosType gpt) { if (gpt.renderJsonAsVespa8()) { serializeGeoPos(generator, field, value, gpt); return; diff --git a/document/src/main/java/com/yahoo/document/serialization/XmlSerializationHelper.java b/document/src/main/java/com/yahoo/document/serialization/XmlSerializationHelper.java index 9c1df0cd6c7..d35693f785f 100644 --- a/document/src/main/java/com/yahoo/document/serialization/XmlSerializationHelper.java +++ b/document/src/main/java/com/yahoo/document/serialization/XmlSerializationHelper.java @@ -34,6 +34,8 @@ import java.util.Map; @SuppressWarnings("removal") public class XmlSerializationHelper { + private final static Base64.Encoder base64Encoder = Base64.getEncoder().withoutPadding(); + public static void printArrayXml(Array array, XmlStream xml) { List<FieldValue> lst = array.getValues(); for (FieldValue value : lst) { @@ -98,7 +100,7 @@ public class XmlSerializationHelper { public static void printRawXml(Raw r, XmlStream xml) { xml.addAttribute("binaryencoding", "base64"); - xml.addContent(Base64.getEncoder().encodeToString(r.getByteBuffer().array())); + xml.addContent(base64Encoder.encodeToString(r.getByteBuffer().array())); } public static void printStringXml(StringFieldValue s, XmlStream xml) { @@ -106,7 +108,7 @@ public class XmlSerializationHelper { if (containsNonPrintableCharactersString(content)) { byte[] bytecontent = Utf8.toBytes(content); xml.addAttribute("binaryencoding", "base64"); - xml.addContent(Base64.getEncoder().encodeToString(bytecontent)); + xml.addContent(base64Encoder.encodeToString(bytecontent)); } else { xml.addContent(content); } diff --git a/document/src/test/java/com/yahoo/document/DocumentTestCase.java b/document/src/test/java/com/yahoo/document/DocumentTestCase.java index 33b77cb1878..4470865b636 100644 --- a/document/src/test/java/com/yahoo/document/DocumentTestCase.java +++ b/document/src/test/java/com/yahoo/document/DocumentTestCase.java @@ -52,7 +52,7 @@ public class DocumentTestCase extends DocumentTestCaseBase { " <mailid>emailfromalicetobob&someone</mailid>\n" + " <date>-2013512400</date>\n" + " <attachmentcount>2</attachmentcount>\n" + - " <rawfield binaryencoding=\"base64\">AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5fYGFiYw==</rawfield>\n"; + " <rawfield binaryencoding=\"base64\">AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5fYGFiYw</rawfield>\n"; private static final String SERTEST_DOC_AS_XML_WEIGHT1 = " <weightedfield>\n" + diff --git a/document/src/test/java/com/yahoo/document/json/DocumentUpdateJsonSerializerTest.java b/document/src/test/java/com/yahoo/document/json/DocumentUpdateJsonSerializerTest.java index 08a5c9a124c..af7469de31b 100644 --- a/document/src/test/java/com/yahoo/document/json/DocumentUpdateJsonSerializerTest.java +++ b/document/src/test/java/com/yahoo/document/json/DocumentUpdateJsonSerializerTest.java @@ -504,7 +504,7 @@ public class DocumentUpdateJsonSerializerTest { " 'update': 'DOCUMENT_ID',", " 'fields': {", " 'raw_field': {", - " 'assign': 'RG9uJ3QgYmVsaWV2ZSBoaXMgbGllcw=='", + " 'assign': 'RG9uJ3QgYmVsaWV2ZSBoaXMgbGllcw'", " }", " }", "}" diff --git a/document/src/test/java/com/yahoo/document/json/JsonReaderTestCase.java b/document/src/test/java/com/yahoo/document/json/JsonReaderTestCase.java index 0c130ab9a42..a761a9adfb6 100644 --- a/document/src/test/java/com/yahoo/document/json/JsonReaderTestCase.java +++ b/document/src/test/java/com/yahoo/document/json/JsonReaderTestCase.java @@ -668,7 +668,7 @@ public class JsonReaderTestCase { @Test public void testRaw() throws IOException { String base64 = new String(new JsonStringEncoder().quoteAsString( - Base64.getEncoder().encodeToString(Utf8.toBytes("smoketest")))); + Base64.getEncoder().withoutPadding().encodeToString(Utf8.toBytes("smoketest")))); String s = fieldStringFromBase64RawContent(base64); assertEquals("smoketest", s); } diff --git a/document/src/test/java/com/yahoo/document/json/JsonWriterTestCase.java b/document/src/test/java/com/yahoo/document/json/JsonWriterTestCase.java index eab33afc3e4..4f15a2fe368 100644 --- a/document/src/test/java/com/yahoo/document/json/JsonWriterTestCase.java +++ b/document/src/test/java/com/yahoo/document/json/JsonWriterTestCase.java @@ -291,7 +291,7 @@ public class JsonWriterTestCase { String payload = new String( new JsonStringEncoder().quoteAsString( "c3RyaW5nIGxvbmcgZW5vdWdoIHRvIGVtaXQgbW9yZSB0aGFuIDc2IGJhc2U2NCBjaGFyYWN0ZXJzIGFuZC" + - "B3aGljaCBzaG91bGQgY2VydGFpbmx5IG5vdCBiZSBuZXdsaW5lLWRlbGltaXRlZCE=")); + "B3aGljaCBzaG91bGQgY2VydGFpbmx5IG5vdCBiZSBuZXdsaW5lLWRlbGltaXRlZCE")); String docId = "id:unittest:testraw::whee"; diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/DropDocumentsReport.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/DropDocumentsReport.java new file mode 100644 index 00000000000..0d88f10ebf9 --- /dev/null +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/DropDocumentsReport.java @@ -0,0 +1,55 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.node.admin.configserver.noderepository.reports; + +import com.fasterxml.jackson.annotation.JsonGetter; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * @author freva + */ +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonInclude(JsonInclude.Include.NON_NULL) +public class DropDocumentsReport extends BaseReport { + private static final String REPORT_ID = "dropDocuments"; + private static final String DROPPED_AT_FIELD = "droppedAt"; + private static final String READIED_AT_FIELD = "readiedAt"; + private static final String STARTED_AT_FIELD = "startedAt"; + + private final Long droppedAt; + private final Long readiedAt; + private final Long startedAt; + + public DropDocumentsReport(@JsonProperty(CREATED_FIELD) Long createdMillisOrNull, + @JsonProperty(DROPPED_AT_FIELD) Long droppedAtOrNull, + @JsonProperty(READIED_AT_FIELD) Long readiedAtOrNull, + @JsonProperty(STARTED_AT_FIELD) Long startedAtOrNull) { + super(createdMillisOrNull, null); + this.droppedAt = droppedAtOrNull; + this.readiedAt = readiedAtOrNull; + this.startedAt = startedAtOrNull; + } + + @JsonGetter(DROPPED_AT_FIELD) + public Long droppedAt() { return droppedAt; } + + @JsonGetter(READIED_AT_FIELD) + public Long readiedAt() { return readiedAt; } + + @JsonGetter(STARTED_AT_FIELD) + public Long startedAt() { return startedAt; } + + public DropDocumentsReport withDroppedAt(long droppedAt) { + return new DropDocumentsReport(getCreatedMillisOrNull(), droppedAt, readiedAt, startedAt); + } + + public DropDocumentsReport withStartedAt(long startedAt) { + return new DropDocumentsReport(getCreatedMillisOrNull(), droppedAt, readiedAt, startedAt); + } + + public static String reportId() { + return REPORT_ID; + } + +} diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java index d22fd667202..3fb9c73367d 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java @@ -122,7 +122,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { Files.createDirectories(privateKeyFile.getParent()); Files.createDirectories(certificateFile.getParent()); Files.createDirectories(identityDocumentFile.getParent()); - registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType); + registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType, athenzIdentity); return true; } @@ -132,11 +132,11 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { var doc = EntityBindingsMapper.readSignedIdentityDocumentFromFile(identityDocumentFile); if (doc.outdated()) { context.log(logger, "Identity document is outdated (version=%d)", doc.documentVersion()); - registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType); + registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType, athenzIdentity); return true; } else if (isCertificateExpired(expiry, now)) { context.log(logger, "Certificate has expired (expiry=%s)", expiry.toString()); - registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType); + registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType, athenzIdentity); return true; } @@ -150,7 +150,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { return false; } else { lastRefreshAttempt.put(context.containerName(), now); - refreshIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, doc, identityType); + refreshIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, doc, identityType, athenzIdentity); return true; } } @@ -198,12 +198,12 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { now)) > 0; } - private void registerIdentity(NodeAgentContext context, ContainerPath privateKeyFile, ContainerPath certificateFile, ContainerPath identityDocumentFile, IdentityType identityType) { + private void registerIdentity(NodeAgentContext context, ContainerPath privateKeyFile, ContainerPath certificateFile, ContainerPath identityDocumentFile, IdentityType identityType, AthenzIdentity identity) { KeyPair keyPair = KeyUtils.generateKeypair(KeyAlgorithm.RSA); SignedIdentityDocument doc = signedIdentityDocument(context, identityType); CsrGenerator csrGenerator = new CsrGenerator(certificateDnsSuffix, doc.providerService().getFullName()); Pkcs10Csr csr = csrGenerator.generateInstanceCsr( - context.identity(), doc.providerUniqueId(), doc.ipAddresses(), doc.clusterType(), keyPair); + identity, doc.providerUniqueId(), doc.ipAddresses(), doc.clusterType(), keyPair); // Allow all zts hosts while removing SIS HostnameVerifier ztsHostNameVerifier = (hostname, sslSession) -> true; @@ -211,7 +211,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { InstanceIdentity instanceIdentity = ztsClient.registerInstance( doc.providerService(), - context.identity(), + identity, EntityBindingsMapper.toAttestationData(doc), csr); EntityBindingsMapper.writeSignedIdentityDocumentToFile(identityDocumentFile, doc); @@ -230,11 +230,11 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { .orElse(ztsEndpoint); } private void refreshIdentity(NodeAgentContext context, ContainerPath privateKeyFile, ContainerPath certificateFile, - ContainerPath identityDocumentFile, SignedIdentityDocument doc, IdentityType identityType) { + ContainerPath identityDocumentFile, SignedIdentityDocument doc, IdentityType identityType, AthenzIdentity identity) { KeyPair keyPair = KeyUtils.generateKeypair(KeyAlgorithm.RSA); CsrGenerator csrGenerator = new CsrGenerator(certificateDnsSuffix, doc.providerService().getFullName()); Pkcs10Csr csr = csrGenerator.generateInstanceCsr( - context.identity(), doc.providerUniqueId(), doc.ipAddresses(), doc.clusterType(), keyPair); + identity, doc.providerUniqueId(), doc.ipAddresses(), doc.clusterType(), keyPair); SSLContext containerIdentitySslContext = new SslContextBuilder().withKeyStore(privateKeyFile, certificateFile) .withTrustStore(ztsTrustStorePath) @@ -247,7 +247,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { InstanceIdentity instanceIdentity = ztsClient.refreshInstance( doc.providerService(), - context.identity(), + identity, doc.providerUniqueId().asDottedString(), csr); writePrivateKeyAndCertificate(privateKeyFile, keyPair.getPrivate(), certificateFile, instanceIdentity.certificate()); @@ -255,7 +255,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { } catch (ZtsClientException e) { if (e.getErrorCode() == 403 && e.getDescription().startsWith("Certificate revoked")) { context.log(logger, Level.SEVERE, "Certificate cannot be refreshed as it is revoked by ZTS - re-registering the instance now", e); - registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType); + registerIdentity(context, privateKeyFile, certificateFile, identityDocumentFile, identityType, identity); } else { throw e; } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java index 20359410321..f2f690106fa 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java @@ -16,6 +16,7 @@ import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeMembers import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeState; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.reports.DropDocumentsReport; import com.yahoo.vespa.hosted.node.admin.configserver.orchestrator.Orchestrator; import com.yahoo.vespa.hosted.node.admin.configserver.orchestrator.OrchestratorException; import com.yahoo.vespa.hosted.node.admin.container.Container; @@ -29,6 +30,7 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer; import com.yahoo.vespa.hosted.node.admin.maintenance.identity.CredentialsMaintainer; import com.yahoo.vespa.hosted.node.admin.maintenance.servicedump.VespaServiceDumper; import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException; +import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder; import java.time.Clock; import java.time.Duration; @@ -228,6 +230,12 @@ public class NodeAgentImpl implements NodeAgent { changed = true; } + Optional<DropDocumentsReport> report = context.node().reports().getReport(DropDocumentsReport.reportId(), DropDocumentsReport.class); + if (report.isPresent() && report.get().startedAt() == null && report.get().readiedAt() != null) { + newNodeAttributes.withReport(DropDocumentsReport.reportId(), report.get().withStartedAt(clock.millis()).toJsonNode()); + changed = true; + } + if (changed) { context.log(logger, "Publishing new set of attributes to node repo: %s -> %s", currentNodeAttributes, newNodeAttributes); @@ -433,6 +441,21 @@ public class NodeAgentImpl implements NodeAgent { .orElse(false); } + private void dropDocsIfNeeded(NodeAgentContext context, Optional<Container> container) { + Optional<DropDocumentsReport> report = context.node().reports() + .getReport(DropDocumentsReport.reportId(), DropDocumentsReport.class); + if (report.isEmpty() || report.get().readiedAt() != null) return; + + if (report.get().droppedAt() == null) { + container.ifPresent(c -> removeContainer(context, c, List.of("Dropping documents"), true)); + FileFinder.from(context.paths().underVespaHome("var/db/vespa/search")).deleteRecursively(context); + nodeRepository.updateNodeAttributes(context.node().hostname(), + new NodeAttributes().withReport(DropDocumentsReport.reportId(), report.get().withDroppedAt(clock.millis()).toJsonNode())); + } + + throw ConvergenceException.ofTransient("Documents already dropped, waiting for signal to start the container"); + } + public void converge(NodeAgentContext context) { try { doConverge(context); @@ -494,6 +517,7 @@ public class NodeAgentImpl implements NodeAgent { context.log(logger, "Waiting for image to download " + context.node().wantedDockerImage().get().asString()); return; } + dropDocsIfNeeded(context, container); container = removeContainerIfNeededUpdateContainerState(context, container); credentialsMaintainers.forEach(maintainer -> maintainer.converge(context)); if (container.isEmpty()) { diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java index b8b72308bdd..2db5314dbf2 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java @@ -14,6 +14,7 @@ import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeReposit import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeState; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.OrchestratorStatus; +import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.reports.DropDocumentsReport; import com.yahoo.vespa.hosted.node.admin.configserver.orchestrator.Orchestrator; import com.yahoo.vespa.hosted.node.admin.configserver.orchestrator.OrchestratorException; import com.yahoo.vespa.hosted.node.admin.container.Container; @@ -27,6 +28,7 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer; import com.yahoo.vespa.hosted.node.admin.maintenance.identity.CredentialsMaintainer; import com.yahoo.vespa.hosted.node.admin.maintenance.servicedump.VespaServiceDumper; import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException; +import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; import com.yahoo.vespa.test.file.TestFileSystem; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -38,8 +40,11 @@ import java.time.Instant; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.BiFunction; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -739,6 +744,56 @@ public class NodeAgentImplTest { inOrder.verify(orchestrator, times(1)).resume(eq(hostName)); } + @Test + void drop_all_documents() { + InOrder inOrder = inOrder(orchestrator, nodeRepository); + BiFunction<NodeState, DropDocumentsReport, NodeSpec> specBuilder = (state, report) -> (report == null ? + nodeBuilder(state) : nodeBuilder(state).report(DropDocumentsReport.reportId(), report.toJsonNode())) + .wantedDockerImage(dockerImage).currentDockerImage(dockerImage) + .build(); + NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true, Duration.ofSeconds(30)); + + NodeAgentContext context = createContext(specBuilder.apply(NodeState.active, null)); + UnixPath indexPath = new UnixPath(context.paths().underVespaHome("var/db/vespa/search/cluster.foo/0/doc")).createParents().createNewFile(); + mockGetContainer(dockerImage, ContainerResources.from(2, 2, 16), true); + assertTrue(indexPath.exists()); + + // Initially no changes, index is not dropped + nodeAgent.converge(context); + assertTrue(indexPath.exists()); + inOrder.verifyNoMoreInteractions(); + + context = createContext(specBuilder.apply(NodeState.active, new DropDocumentsReport(1L, null, null, null))); + nodeAgent.converge(context); + verify(containerOperations).removeContainer(eq(context), any()); + assertFalse(indexPath.exists()); + inOrder.verify(nodeRepository).updateNodeAttributes(eq(hostName), eq(new NodeAttributes().withReport(DropDocumentsReport.reportId(), new DropDocumentsReport(1L, clock.millis(), null, null).toJsonNode()))); + inOrder.verifyNoMoreInteractions(); + + // After droppedAt and before readiedAt are set, we cannot proceed + mockGetContainer(null, false); + context = createContext(specBuilder.apply(NodeState.active, new DropDocumentsReport(1L, 2L, null, null))); + nodeAgent.converge(context); + verify(containerOperations, never()).removeContainer(eq(context), any()); + verify(containerOperations, never()).startContainer(eq(context)); + inOrder.verifyNoMoreInteractions(); + + context = createContext(specBuilder.apply(NodeState.active, new DropDocumentsReport(1L, 2L, 3L, null))); + nodeAgent.converge(context); + verify(containerOperations).startContainer(eq(context)); + inOrder.verifyNoMoreInteractions(); + + mockGetContainer(dockerImage, ContainerResources.from(0, 2, 16), true); + clock.advance(Duration.ofSeconds(31)); + nodeAgent.converge(context); + verify(containerOperations, times(1)).startContainer(eq(context)); + verify(containerOperations, never()).removeContainer(eq(context), any()); + inOrder.verify(nodeRepository).updateNodeAttributes(eq(hostName), eq(new NodeAttributes() + .withRebootGeneration(0) + .withReport(DropDocumentsReport.reportId(), new DropDocumentsReport(1L, 2L, 3L, clock.millis()).toJsonNode()))); + inOrder.verifyNoMoreInteractions(); + } + private void verifyThatContainerIsStopped(NodeState nodeState, Optional<ApplicationId> owner) { NodeSpec.Builder nodeBuilder = nodeBuilder(nodeState) .type(NodeType.tenant) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java index dfe01f5f1c3..bbe287fc034 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodePatcher.java @@ -11,8 +11,10 @@ import com.yahoo.config.provision.NodeFlavors; import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.WireguardKey; +import com.yahoo.slime.Cursor; import com.yahoo.slime.Inspector; import com.yahoo.slime.ObjectTraverser; +import com.yahoo.slime.Slime; import com.yahoo.slime.SlimeUtils; import com.yahoo.slime.Type; import com.yahoo.vespa.hosted.provision.LockedNodeList; @@ -40,6 +42,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.TreeSet; +import java.util.stream.Stream; import static com.yahoo.config.provision.NodeResources.DiskSpeed.fast; import static com.yahoo.config.provision.NodeResources.DiskSpeed.slow; @@ -54,9 +57,13 @@ import static com.yahoo.config.provision.NodeResources.StorageType.remote; */ public class NodePatcher { + // Same as in DropDocumentsReport.java + private static final String DROP_DOCUMENTS_REPORT = "dropDocuments"; + private static final String WANT_TO_RETIRE = "wantToRetire"; private static final String WANT_TO_DEPROVISION = "wantToDeprovision"; private static final String WANT_TO_REBUILD = "wantToRebuild"; + private static final String REPORTS = "reports"; private static final Set<String> RECURSIVE_FIELDS = Set.of(WANT_TO_RETIRE, WANT_TO_DEPROVISION); private static final Set<String> IP_CONFIG_FIELDS = Set.of("ipAddresses", "additionalIpAddresses", @@ -133,7 +140,29 @@ public class NodePatcher { throw new IllegalArgumentException("Could not set field '" + name + "'", e); } } - nodeRepository.nodes().write(node, lock); + List<Node> nodes = List.of(node); + if (node.state() == Node.State.active && isInDocumentsDroppedState(root.field(REPORTS).field(DROP_DOCUMENTS_REPORT))) { + NodeList clusterNodes = nodeRepository.nodes() + .list(Node.State.active) + .except(node) + .owner(node.allocation().get().owner()) + .cluster(node.allocation().get().membership().cluster().id()); + boolean allNodesDroppedDocuments = clusterNodes.stream().allMatch(cNode -> + cNode.reports().getReport(DROP_DOCUMENTS_REPORT).map(report -> isInDocumentsDroppedState(report.getInspector())).orElse(false)); + if (allNodesDroppedDocuments) { + nodes = Stream.concat(nodes.stream(), clusterNodes.stream()) + .map(cNode -> { + Cursor reportRoot = new Slime().setObject(); + Report report = cNode.reports().getReport(DROP_DOCUMENTS_REPORT).get(); + report.toSlime(reportRoot); + reportRoot.setLong("readiedAt", clock.millis()); + + return cNode.with(cNode.reports().withReport(Report.fromSlime(DROP_DOCUMENTS_REPORT, reportRoot))); + }) + .toList(); + } + } + nodeRepository.nodes().write(nodes, lock); } } @@ -202,18 +231,15 @@ public class NodePatcher { .orElseGet(node.status()::wantToRebuild), Agent.operator, clock.instant()); - case "reports" : + case REPORTS: return nodeWithPatchedReports(node, value); - case "id" : + case "id": return node.withId(asString(value)); case "diskGb": - case "minDiskAvailableGb": return node.with(node.flavor().with(node.flavor().resources().withDiskGb(value.asDouble())), Agent.operator, clock.instant()); case "memoryGb": - case "minMainMemoryAvailableGb": return node.with(node.flavor().with(node.flavor().resources().withMemoryGb(value.asDouble())), Agent.operator, clock.instant()); case "vcpu": - case "minCpuCores": return node.with(node.flavor().with(node.flavor().resources().withVcpu(value.asDouble())), Agent.operator, clock.instant()); case "fastDisk": return node.with(node.flavor().with(node.flavor().resources().with(value.asBool() ? fast : slow)), Agent.operator, clock.instant()); @@ -244,18 +270,12 @@ public class NodePatcher { } private Node applyIpconfigField(Node node, String name, Inspector value, LockedNodeList nodes) { - switch (name) { - case "ipAddresses" -> { - return IP.Config.verify(node.with(node.ipConfig().withPrimary(asStringSet(value))), nodes); - } - case "additionalIpAddresses" -> { - return IP.Config.verify(node.with(node.ipConfig().withPool(node.ipConfig().pool().withIpAddresses(asStringSet(value)))), nodes); - } - case "additionalHostnames" -> { - return IP.Config.verify(node.with(node.ipConfig().withPool(node.ipConfig().pool().withHostnames(asHostnames(value)))), nodes); - } - } - throw new IllegalArgumentException("Could not apply field '" + name + "' on a node: No such modifiable field"); + return switch (name) { + case "ipAddresses" -> IP.Config.verify(node.with(node.ipConfig().withPrimary(asStringSet(value))), nodes); + case "additionalIpAddresses" -> IP.Config.verify(node.with(node.ipConfig().withPool(node.ipConfig().pool().withIpAddresses(asStringSet(value)))), nodes); + case "additionalHostnames" -> IP.Config.verify(node.with(node.ipConfig().withPool(node.ipConfig().pool().withHostnames(asHostnames(value)))), nodes); + default -> throw new IllegalArgumentException("Could not apply field '" + name + "' on a node: No such modifiable field"); + }; } private Node nodeWithPatchedReports(Node node, Inspector reportsInspector) { @@ -374,4 +394,9 @@ public class NodePatcher { return Optional.of(field).filter(Inspector::valid).map(this::asBoolean); } + private static boolean isInDocumentsDroppedState(Inspector report) { + if (!report.valid()) return false; + return report.field("droppedAt").valid() && !report.field("readiedAt").valid(); + } + } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java index c9e57c22d11..7affcfebdb3 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiTest.java @@ -647,6 +647,22 @@ public class NodesV2ApiTest { Request.Method.PATCH), "{\"message\":\"Updated dockerhost1.yahoo.com\"}"); assertFile(new Request("http://localhost:8080/nodes/v2/node/dockerhost1.yahoo.com"), "docker-node1-reports-4.json"); + + assertResponse(new Request("http://localhost:8080/nodes/v2/node/host1.yahoo.com", + Utf8.toBytes("{\"reports\": {\"dropDocuments\":{\"createdMillis\":25,\"droppedAt\":36}}}"), + Request.Method.PATCH), + "{\"message\":\"Updated host1.yahoo.com\"}"); + tester.assertResponseContains(new Request("http://localhost:8080/nodes/v2/node/host1.yahoo.com"), + "{\"dropDocuments\":{\"createdMillis\":25,\"droppedAt\":36}}"); + + assertResponse(new Request("http://localhost:8080/nodes/v2/node/host10.yahoo.com", + Utf8.toBytes("{\"reports\": {\"dropDocuments\":{\"createdMillis\":49,\"droppedAt\":456}}}"), + Request.Method.PATCH), + "{\"message\":\"Updated host10.yahoo.com\"}"); + tester.assertResponseContains(new Request("http://localhost:8080/nodes/v2/node/host10.yahoo.com"), + "{\"dropDocuments\":{\"createdMillis\":49,\"droppedAt\":456,\"readiedAt\":123}}"); + tester.assertResponseContains(new Request("http://localhost:8080/nodes/v2/node/host1.yahoo.com"), + "{\"dropDocuments\":{\"createdMillis\":25,\"droppedAt\":36,\"readiedAt\":123}}"); } @Test @@ -906,13 +922,13 @@ public class NodesV2ApiTest { // Test patching with overrides tester.assertResponse(new Request("http://localhost:8080/nodes/v2/node/" + host, - "{\"minDiskAvailableGb\":5432,\"minMainMemoryAvailableGb\":2345}".getBytes(StandardCharsets.UTF_8), + "{\"diskGb\":5432,\"memoryGb\":2345}".getBytes(StandardCharsets.UTF_8), Request.Method.PATCH), 400, - "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Could not set field 'minMainMemoryAvailableGb': Can only override disk GB for configured flavor\"}"); + "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Could not set field 'memoryGb': Can only override disk GB for configured flavor\"}"); assertResponse(new Request("http://localhost:8080/nodes/v2/node/" + host, - "{\"minDiskAvailableGb\":5432}".getBytes(StandardCharsets.UTF_8), + "{\"diskGb\":5432}".getBytes(StandardCharsets.UTF_8), Request.Method.PATCH), "{\"message\":\"Updated " + host + "\"}"); tester.assertResponseContains(new Request("http://localhost:8080/nodes/v2/node/" + host), diff --git a/searchcore/src/tests/proton/attribute/attribute_initializer/attribute_initializer_test.cpp b/searchcore/src/tests/proton/attribute/attribute_initializer/attribute_initializer_test.cpp index 4af23a1d7fb..d2798c16065 100644 --- a/searchcore/src/tests/proton/attribute/attribute_initializer/attribute_initializer_test.cpp +++ b/searchcore/src/tests/proton/attribute/attribute_initializer/attribute_initializer_test.cpp @@ -277,11 +277,7 @@ TEST("require that reserved document is reinitialized during load") auto read_view = mvav->make_read_view(IMultiValueAttribute::WeightedSetTag<const char*>(), stash); ASSERT_TRUE(read_view != nullptr); auto reserved_values = read_view->get_values(0u); - EXPECT_EQUAL(1u, reserved_values.size()); - if (reserved_values.size() >= 1) { - EXPECT_EQUAL(1, reserved_values[0].weight()); - EXPECT_EQUAL(vespalib::string(""), vespalib::string(reserved_values[0].value())); - } + EXPECT_EQUAL(0u, reserved_values.size()); } } diff --git a/searchlib/src/tests/query/streaming_query_test.cpp b/searchlib/src/tests/query/streaming_query_test.cpp index f354f635def..2c202d9131b 100644 --- a/searchlib/src/tests/query/streaming_query_test.cpp +++ b/searchlib/src/tests/query/streaming_query_test.cpp @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include <vespa/searchlib/query/streaming/query.h> +#include <vespa/searchlib/query/streaming/nearest_neighbor_query_node.h> #include <vespa/searchlib/query/tree/querybuilder.h> #include <vespa/searchlib/query/tree/simplequery.h> #include <vespa/searchlib/query/tree/stackdumpcreator.h> @@ -804,6 +805,42 @@ TEST("testSameElementEvaluate") { EXPECT_TRUE(sameElem->evaluate()); } +TEST("test_nearest_neighbor_query_node") +{ + QueryBuilder<SimpleQueryNodeTypes> builder; + constexpr double distance_threshold = 35.5; + constexpr int32_t id = 42; + constexpr int32_t weight = 1; + constexpr uint32_t target_num_hits = 100; + constexpr bool allow_approximate = false; + constexpr uint32_t explore_additional_hits = 800; + constexpr double raw_score = 0.5; + builder.add_nearest_neighbor_term("qtensor", "field", id, Weight(weight), target_num_hits, allow_approximate, explore_additional_hits, distance_threshold); + auto build_node = builder.build(); + auto stack_dump = StackDumpCreator::create(*build_node); + QueryNodeResultFactory empty; + Query q(empty, stack_dump); + auto* qterm = dynamic_cast<QueryTerm *>(&q.getRoot()); + EXPECT_TRUE(qterm != nullptr); + auto* node = dynamic_cast<NearestNeighborQueryNode *>(&q.getRoot()); + EXPECT_TRUE(node != nullptr); + EXPECT_EQUAL(node, qterm->as_nearest_neighbor_query_node()); + EXPECT_EQUAL("qtensor", node->get_query_tensor_name()); + EXPECT_EQUAL("field", node->getIndex()); + EXPECT_EQUAL(id, static_cast<int32_t>(node->uniqueId())); + EXPECT_EQUAL(weight, node->weight().percent()); + EXPECT_EQUAL(distance_threshold, node->get_distance_threshold()); + EXPECT_FALSE(node->get_raw_score().has_value()); + EXPECT_FALSE(node->evaluate()); + node->set_raw_score(raw_score); + EXPECT_TRUE(node->get_raw_score().has_value()); + EXPECT_EQUAL(raw_score, node->get_raw_score().value()); + EXPECT_TRUE(node->evaluate()); + node->reset(); + EXPECT_FALSE(node->get_raw_score().has_value()); + EXPECT_FALSE(node->evaluate()); +} + TEST("Control the size of query terms") { EXPECT_EQUAL(112u, sizeof(QueryTermSimple)); EXPECT_EQUAL(128u, sizeof(QueryTermUCS4)); diff --git a/searchlib/src/vespa/searchlib/attribute/attributevector.cpp b/searchlib/src/vespa/searchlib/attribute/attributevector.cpp index 9110c08099a..f4ab447ed51 100644 --- a/searchlib/src/vespa/searchlib/attribute/attributevector.cpp +++ b/searchlib/src/vespa/searchlib/attribute/attributevector.cpp @@ -453,19 +453,6 @@ AttributeVector::set_reserved_doc_values() return; } clearDoc(docId); - if (hasMultiValue()) { - if (isFloatingPointType()) { - auto * vec = dynamic_cast<FloatingPointAttribute *>(this); - bool appendedUndefined = vec->append(0, attribute::getUndefined<double>(), 1); - assert(appendedUndefined); - (void) appendedUndefined; - } else if (isStringType()) { - auto * vec = dynamic_cast<StringAttribute *>(this); - bool appendedUndefined = vec->append(0, StringAttribute::defaultValue(), 1); - assert(appendedUndefined); - (void) appendedUndefined; - } - } commit(); } diff --git a/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.cpp b/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.cpp index fdc513f9617..d1c37cd6dcd 100644 --- a/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.cpp +++ b/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.cpp @@ -6,7 +6,8 @@ namespace search::streaming { NearestNeighborQueryNode::NearestNeighborQueryNode(std::unique_ptr<QueryNodeResultBase> resultBase, const string& term, const string& index, int32_t id, search::query::Weight weight, double distance_threshold) : QueryTerm(std::move(resultBase), term, index, Type::NEAREST_NEIGHBOR), - _distance_threshold(distance_threshold) + _distance_threshold(distance_threshold), + _raw_score() { setUniqueId(id); setWeight(weight); @@ -14,6 +15,18 @@ NearestNeighborQueryNode::NearestNeighborQueryNode(std::unique_ptr<QueryNodeResu NearestNeighborQueryNode::~NearestNeighborQueryNode() = default; +bool +NearestNeighborQueryNode::evaluate() const +{ + return _raw_score.has_value(); +} + +void +NearestNeighborQueryNode::reset() +{ + _raw_score.reset(); +} + NearestNeighborQueryNode* NearestNeighborQueryNode::as_nearest_neighbor_query_node() noexcept { diff --git a/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.h b/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.h index ddc84a4b6d3..0beb130c53d 100644 --- a/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.h +++ b/searchlib/src/vespa/searchlib/query/streaming/nearest_neighbor_query_node.h @@ -3,15 +3,19 @@ #pragma once #include "queryterm.h" +#include <optional> namespace search::streaming { /* * Nearest neighbor query node. */ -class NearestNeighborQueryNode: public QueryTerm -{ - double _distance_threshold; +class NearestNeighborQueryNode: public QueryTerm { +private: + double _distance_threshold; + // When this value is set it also indicates a match + std::optional<double> _raw_score; + public: NearestNeighborQueryNode(std::unique_ptr<QueryNodeResultBase> resultBase, const string& term, const string& index, int32_t id, search::query::Weight weight, double distance_threshold); NearestNeighborQueryNode(const NearestNeighborQueryNode &) = delete; @@ -19,9 +23,13 @@ public: NearestNeighborQueryNode(NearestNeighborQueryNode &&) = delete; NearestNeighborQueryNode & operator = (NearestNeighborQueryNode &&) = delete; ~NearestNeighborQueryNode() override; + bool evaluate() const override; + void reset() override; NearestNeighborQueryNode* as_nearest_neighbor_query_node() noexcept override; const vespalib::string& get_query_tensor_name() const { return getTermString(); } double get_distance_threshold() const { return _distance_threshold; } + void set_raw_score(double value) { _raw_score = value; } + const std::optional<double>& get_raw_score() const noexcept { return _raw_score; } }; } diff --git a/storage/src/tests/persistence/persistencetestutils.h b/storage/src/tests/persistence/persistencetestutils.h index 94ae7b9fb53..e60260f3ee8 100644 --- a/storage/src/tests/persistence/persistencetestutils.h +++ b/storage/src/tests/persistence/persistencetestutils.h @@ -150,6 +150,18 @@ public: _replySender, MockBucketLock::make(bucket, _mock_bucket_locks), std::move(cmd)); } + template <typename T> + requires std::is_base_of_v<api::StorageReply, T> + [[nodiscard]] std::shared_ptr<T> + fetch_single_reply(MessageTracker::UP tracker) { + if (tracker && tracker->hasReply()) { + tracker->sendReply(); // Forward to queue so we can fetch it below + } + std::shared_ptr<api::StorageMessage> msg; + _replySender.queue.getNext(msg, 60s); + return std::dynamic_pointer_cast<T>(msg); + } + api::ReturnCode fetchResult(const MessageTracker::UP & tracker) { if (tracker) { diff --git a/storage/src/tests/persistence/testandsettest.cpp b/storage/src/tests/persistence/testandsettest.cpp index 5be1c7cd92a..1aa359de634 100644 --- a/storage/src/tests/persistence/testandsettest.cpp +++ b/storage/src/tests/persistence/testandsettest.cpp @@ -1,16 +1,16 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. // @author Vegard Sjonfjell -#include <vespa/storage/persistence/persistencehandler.h> #include <tests/persistence/persistencetestutils.h> #include <vespa/document/test/make_document_bucket.h> -#include <vespa/documentapi/messagebus/messages/testandsetcondition.h> #include <vespa/document/fieldvalue/fieldvalues.h> #include <vespa/document/update/documentupdate.h> #include <vespa/document/update/assignvalueupdate.h> #include <vespa/document/fieldset/fieldsets.h> +#include <vespa/documentapi/messagebus/messages/testandsetcondition.h> #include <vespa/persistence/spi/test.h> #include <vespa/persistence/spi/persistenceprovider.h> #include <vespa/persistence/spi/docentry.h> +#include <vespa/storage/persistence/persistencehandler.h> #include <functional> using std::unique_ptr; @@ -19,6 +19,7 @@ using std::shared_ptr; using storage::spi::test::makeSpiBucket; using document::test::makeDocumentBucket; using document::StringFieldValue; +using documentapi::TestAndSetCondition; using namespace ::testing; namespace storage { @@ -34,15 +35,18 @@ struct TestAndSetTest : PersistenceTestUtils { const StringFieldValue OLD_CONTENT{"Some old content"}; const StringFieldValue NEW_CONTENT{"Freshly pressed and squeezed content"}; const document::Bucket BUCKET = makeDocumentBucket(BUCKET_ID); + const TestAndSetCondition MATCHING_CONDITION{"testdoctype1.hstringval=\"*woofy dog*\""}; unique_ptr<PersistenceHandler> persistenceHandler; const AsyncHandler * asyncHandler; + const SimpleMessageHandler* simple_handler; shared_ptr<document::Document> testDoc; document::DocumentId testDocId; TestAndSetTest() : persistenceHandler(), - asyncHandler(nullptr) + asyncHandler(nullptr), + simple_handler(nullptr) {} void SetUp() override { @@ -54,6 +58,7 @@ struct TestAndSetTest : PersistenceTestUtils { testDoc = createTestDocument(); testDocId = testDoc->getId(); asyncHandler = &_persistenceHandler->asyncHandler(); + simple_handler = &_persistenceHandler->simpleMessageHandler(); } void TearDown() override { @@ -68,6 +73,8 @@ struct TestAndSetTest : PersistenceTestUtils { document::Document::SP retrieveTestDocument(); void setTestCondition(api::TestAndSetCommand & command); void putTestDocument(bool matchingHeader, api::Timestamp timestamp); + std::shared_ptr<api::GetReply> invoke_conditional_get(); + void feed_remove_entry_with_timestamp(api::Timestamp timestamp); void assertTestDocumentFoundAndMatchesContent(const document::FieldValue & value); static std::string expectedDocEntryString( @@ -247,6 +254,59 @@ TEST_F(TestAndSetTest, conditional_put_to_non_existing_document_should_fail) { EXPECT_EQ("", dumpBucket(BUCKET_ID)); } +TEST_F(TestAndSetTest, conditional_get_returns_doc_metadata_on_match) { + const api::Timestamp timestamp = 12345; + putTestDocument(true, timestamp); + auto reply = invoke_conditional_get(); + + ASSERT_EQ(reply->getResult(), api::ReturnCode()); + EXPECT_EQ(reply->getLastModifiedTimestamp(), timestamp); + EXPECT_TRUE(reply->condition_matched()); + EXPECT_FALSE(reply->is_tombstone()); + // Checking reply->wasFound() is tempting but doesn't make sense here, as that checks for + // the presence of a document object, which metadata-only gets by definition do not return. +} + +TEST_F(TestAndSetTest, conditional_get_returns_doc_metadata_on_mismatch) { + const api::Timestamp timestamp = 12345; + putTestDocument(false, timestamp); + auto reply = invoke_conditional_get(); + + ASSERT_EQ(reply->getResult(), api::ReturnCode()); + EXPECT_EQ(reply->getLastModifiedTimestamp(), timestamp); + EXPECT_FALSE(reply->condition_matched()); + EXPECT_FALSE(reply->is_tombstone()); +} + +TEST_F(TestAndSetTest, conditional_get_for_non_existing_document_returns_zero_timestamp) { + auto reply = invoke_conditional_get(); + + ASSERT_EQ(reply->getResult(), api::ReturnCode()); + EXPECT_EQ(reply->getLastModifiedTimestamp(), 0); + EXPECT_FALSE(reply->condition_matched()); + EXPECT_FALSE(reply->is_tombstone()); +} + +TEST_F(TestAndSetTest, conditional_get_for_non_existing_document_with_explicit_tombstone_returns_tombstone_timestamp) { + api::Timestamp timestamp = 56789; + feed_remove_entry_with_timestamp(timestamp); + auto reply = invoke_conditional_get(); + + ASSERT_EQ(reply->getResult(), api::ReturnCode()); + EXPECT_EQ(reply->getLastModifiedTimestamp(), timestamp); + EXPECT_FALSE(reply->condition_matched()); + EXPECT_TRUE(reply->is_tombstone()); +} + +TEST_F(TestAndSetTest, conditional_get_requires_metadata_only_fieldset) { + auto get = std::make_shared<api::GetCommand>(BUCKET, testDocId, document::AllFields::NAME); + get->set_condition(MATCHING_CONDITION); + // Note: uses fetchResult instead of fetch_single_reply due to implicit failure signalling via tracker instance. + auto result = fetchResult(simple_handler->handleGet(*get, createTracker(get, BUCKET))); + ASSERT_EQ(result, api::ReturnCode(api::ReturnCode::ILLEGAL_PARAMETERS, + "Conditional Get operations must be metadata-only")); +} + document::Document::SP TestAndSetTest::createTestDocument() { @@ -270,7 +330,7 @@ TestAndSetTest::retrieveTestDocument() auto tracker = _persistenceHandler->simpleMessageHandler().handleGet(*get, createTracker(get, BUCKET)); assert(tracker->getResult() == api::ReturnCode::Result::OK); - auto & reply = static_cast<api::GetReply &>(tracker->getReply()); + auto& reply = dynamic_cast<api::GetReply&>(tracker->getReply()); assert(reply.wasFound()); return reply.getDocument(); @@ -278,7 +338,7 @@ TestAndSetTest::retrieveTestDocument() void TestAndSetTest::setTestCondition(api::TestAndSetCommand & command) { - command.setCondition(documentapi::TestAndSetCondition("testdoctype1.hstringval=\"*woofy dog*\"")); + command.setCondition(MATCHING_CONDITION); } void TestAndSetTest::putTestDocument(bool matchingHeader, api::Timestamp timestamp) { @@ -290,6 +350,17 @@ void TestAndSetTest::putTestDocument(bool matchingHeader, api::Timestamp timesta fetchResult(asyncHandler->handlePut(*put, createTracker(put, BUCKET))); } +std::shared_ptr<api::GetReply> TestAndSetTest::invoke_conditional_get() { + auto get = std::make_shared<api::GetCommand>(BUCKET, testDocId, document::NoFields::NAME); + get->set_condition(MATCHING_CONDITION); + return fetch_single_reply<api::GetReply>(simple_handler->handleGet(*get, createTracker(get, BUCKET))); +} + +void TestAndSetTest::feed_remove_entry_with_timestamp(api::Timestamp timestamp) { + auto remove = std::make_shared<api::RemoveCommand>(BUCKET, testDocId, timestamp); + (void)fetchResult(asyncHandler->handleRemove(*remove, createTracker(remove, BUCKET))); +} + void TestAndSetTest::assertTestDocumentFoundAndMatchesContent(const document::FieldValue & value) { auto doc = retrieveTestDocument(); diff --git a/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp b/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp index d3036a2fad3..6d8c3585726 100644 --- a/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp +++ b/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp @@ -848,7 +848,7 @@ TEST_P(StorageProtocolTest, track_memory_footprint_for_some_messages) { EXPECT_EQ(144u + sizeof(vespalib::string), sizeof(PutCommand)); EXPECT_EQ(144u + sizeof(vespalib::string), sizeof(UpdateCommand)); EXPECT_EQ(224u + sizeof(vespalib::string), sizeof(RemoveCommand)); - EXPECT_EQ(296u, sizeof(GetCommand)); + EXPECT_EQ(296u + sizeof(documentapi::TestAndSetCondition), sizeof(GetCommand)); } } // storage::api diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp index e20c0475556..60c6d507416 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.cpp +++ b/storage/src/vespa/storage/persistence/asynchandler.cpp @@ -358,7 +358,11 @@ bool AsyncHandler::tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker, spi::Context & context, bool missingDocumentImpliesMatch) const { try { - TestAndSetHelper helper(_env, _spi, _bucketIdFactory, cmd, missingDocumentImpliesMatch); + TestAndSetHelper helper(_env, _spi, _bucketIdFactory, + cmd.getCondition(), + cmd.getBucket(), cmd.getDocumentId(), + cmd.getDocumentType(), + missingDocumentImpliesMatch); auto code = helper.retrieveAndMatch(context); if (code.failed()) { diff --git a/storage/src/vespa/storage/persistence/persistencehandler.cpp b/storage/src/vespa/storage/persistence/persistencehandler.cpp index 8d71cc9308b..69f910d0910 100644 --- a/storage/src/vespa/storage/persistence/persistencehandler.cpp +++ b/storage/src/vespa/storage/persistence/persistencehandler.cpp @@ -24,7 +24,7 @@ PersistenceHandler::PersistenceHandler(vespalib::ISequencedTaskExecutor & sequen cfg.commonMergeChainOptimalizationMinimumSize), _asyncHandler(_env, provider, bucketOwnershipNotifier, sequencedExecutor, component.getBucketIdFactory()), _splitJoinHandler(_env, provider, bucketOwnershipNotifier, cfg.enableMultibitSplitOptimalization), - _simpleHandler(_env, provider) + _simpleHandler(_env, provider, component.getBucketIdFactory()) { } diff --git a/storage/src/vespa/storage/persistence/simplemessagehandler.cpp b/storage/src/vespa/storage/persistence/simplemessagehandler.cpp index e83d460f47a..ea929bf8620 100644 --- a/storage/src/vespa/storage/persistence/simplemessagehandler.cpp +++ b/storage/src/vespa/storage/persistence/simplemessagehandler.cpp @@ -2,6 +2,7 @@ #include "simplemessagehandler.h" #include "persistenceutil.h" +#include "testandsethelper.h" #include <vespa/persistence/spi/persistenceprovider.h> #include <vespa/persistence/spi/docentry.h> #include <vespa/storageapi/message/bucket.h> @@ -45,21 +46,45 @@ getFieldSet(const document::FieldSetRepo & repo, vespalib::stringref name, Messa } } -SimpleMessageHandler::SimpleMessageHandler(const PersistenceUtil& env, spi::PersistenceProvider& spi) +SimpleMessageHandler::SimpleMessageHandler(const PersistenceUtil& env, + spi::PersistenceProvider& spi, + const document::BucketIdFactory& bucket_id_factory) : _env(env), - _spi(spi) + _spi(spi), + _bucket_id_factory(bucket_id_factory) { } MessageTracker::UP +SimpleMessageHandler::handle_conditional_get(api::GetCommand& cmd, MessageTracker::UP tracker) const +{ + if (cmd.getFieldSet() == document::NoFields::NAME) { + TestAndSetHelper tas_helper(_env, _spi, _bucket_id_factory, cmd.condition(), + cmd.getBucket(), cmd.getDocumentId(), nullptr); + auto result = tas_helper.fetch_and_match_raw(tracker->context()); + tracker->setReply(std::make_shared<api::GetReply>(cmd, nullptr, result.timestamp, false, + result.is_tombstone(), result.is_match())); + } else { + tracker->fail(api::ReturnCode::ILLEGAL_PARAMETERS, "Conditional Get operations must be metadata-only"); + } + return tracker; +} + +MessageTracker::UP SimpleMessageHandler::handleGet(api::GetCommand& cmd, MessageTracker::UP tracker) const { auto& metrics = _env._metrics.get; tracker->setMetric(metrics); metrics.request_size.addValue(cmd.getApproxByteSize()); + if (cmd.has_condition()) { + return handle_conditional_get(cmd, std::move(tracker)); + } + auto fieldSet = getFieldSet(_env.getFieldSetRepo(), cmd.getFieldSet(), *tracker); - if ( ! fieldSet) { return tracker; } + if (!fieldSet) { + return tracker; + } tracker->context().setReadConsistency(api_read_consistency_to_spi(cmd.internal_read_consistency())); spi::GetResult result = _spi.get(_env.getBucket(cmd.getDocumentId(), cmd.getBucket()), @@ -70,7 +95,7 @@ SimpleMessageHandler::handleGet(api::GetCommand& cmd, MessageTracker::UP tracker metrics.notFound.inc(); } tracker->setReply(std::make_shared<api::GetReply>(cmd, result.getDocumentPtr(), result.getTimestamp(), - false, result.is_tombstone())); + false, result.is_tombstone(), false)); } return tracker; diff --git a/storage/src/vespa/storage/persistence/simplemessagehandler.h b/storage/src/vespa/storage/persistence/simplemessagehandler.h index 009fd6dff52..a5a19772556 100644 --- a/storage/src/vespa/storage/persistence/simplemessagehandler.h +++ b/storage/src/vespa/storage/persistence/simplemessagehandler.h @@ -7,6 +7,8 @@ #include <vespa/storage/common/bucketmessages.h> #include <vespa/storageapi/message/persistence.h> +namespace document { class BucketIdFactory; } + namespace storage { namespace spi { struct PersistenceProvider; } @@ -19,7 +21,9 @@ class PersistenceUtil; */ class SimpleMessageHandler : public Types { public: - SimpleMessageHandler(const PersistenceUtil&, spi::PersistenceProvider&); + SimpleMessageHandler(const PersistenceUtil&, + spi::PersistenceProvider&, + const document::BucketIdFactory&); MessageTrackerUP handleGet(api::GetCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleRevert(api::RevertCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleCreateIterator(CreateIteratorCommand& cmd, MessageTrackerUP tracker) const; @@ -27,8 +31,11 @@ public: MessageTrackerUP handleReadBucketList(ReadBucketList& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleReadBucketInfo(ReadBucketInfo& cmd, MessageTrackerUP tracker) const; private: - const PersistenceUtil & _env; - spi::PersistenceProvider & _spi; + MessageTrackerUP handle_conditional_get(api::GetCommand& cmd, MessageTrackerUP tracker) const; + + const PersistenceUtil& _env; + spi::PersistenceProvider& _spi; + const document::BucketIdFactory& _bucket_id_factory; }; } // storage diff --git a/storage/src/vespa/storage/persistence/testandsethelper.cpp b/storage/src/vespa/storage/persistence/testandsethelper.cpp index 393dac09f72..1cda9427761 100644 --- a/storage/src/vespa/storage/persistence/testandsethelper.cpp +++ b/storage/src/vespa/storage/persistence/testandsethelper.cpp @@ -31,69 +31,91 @@ void TestAndSetHelper::parseDocumentSelection(const document::DocumentTypeRepo & document::select::Parser parser(documentTypeRepo, bucketIdFactory); try { - _docSelectionUp = parser.parse(_cmd.getCondition().getSelection()); + _docSelectionUp = parser.parse(_condition.getSelection()); } catch (const document::select::ParsingFailedException & e) { throw TestAndSetException(api::ReturnCode(api::ReturnCode::ILLEGAL_PARAMETERS, "Failed to parse test and set condition: "s + e.getMessage())); } } spi::GetResult TestAndSetHelper::retrieveDocument(const document::FieldSet & fieldSet, spi::Context & context) { - return _spi.get(_env.getBucket(_docId, _cmd.getBucket()), fieldSet, _cmd.getDocumentId(), context); + return _spi.get(_env.getBucket(_docId, _bucket), fieldSet, _docId, context); } -TestAndSetHelper::TestAndSetHelper(const PersistenceUtil & env, const spi::PersistenceProvider & spi, - const document::BucketIdFactory & bucketFactory, - const api::TestAndSetCommand & cmd, bool missingDocumentImpliesMatch) +TestAndSetHelper::TestAndSetHelper(const PersistenceUtil& env, + const spi::PersistenceProvider& spi, + const document::BucketIdFactory& bucket_id_factory, + const documentapi::TestAndSetCondition& condition, + document::Bucket bucket, + document::DocumentId doc_id, + const document::DocumentType* doc_type_ptr, + bool missingDocumentImpliesMatch) : _env(env), _spi(spi), - _cmd(cmd), - _docId(cmd.getDocumentId()), - _docTypePtr(_cmd.getDocumentType()), + _condition(condition), + _bucket(bucket), + _docId(std::move(doc_id)), + _docTypePtr(doc_type_ptr), _missingDocumentImpliesMatch(missingDocumentImpliesMatch) { const auto & repo = _env.getDocumentTypeRepo(); resolveDocumentType(repo); - parseDocumentSelection(repo, bucketFactory); + parseDocumentSelection(repo, bucket_id_factory); } TestAndSetHelper::~TestAndSetHelper() = default; -api::ReturnCode -TestAndSetHelper::retrieveAndMatch(spi::Context & context) { - // Walk document selection tree to build a minimal field set +TestAndSetHelper::Result +TestAndSetHelper::fetch_and_match_raw(spi::Context& context) { + // Walk document selection tree to build a minimal field set FieldVisitor fieldVisitor(*_docTypePtr); try { _docSelectionUp->visit(fieldVisitor); } catch (const document::FieldNotFoundException& e) { - return api::ReturnCode(api::ReturnCode::ILLEGAL_PARAMETERS, - vespalib::make_string("Condition field '%s' could not be found, or is an imported field. " - "Imported fields are not supported in conditional mutations.", - e.getFieldName().c_str())); + throw TestAndSetException(api::ReturnCode( + api::ReturnCode::ILLEGAL_PARAMETERS, + vespalib::make_string("Condition field '%s' could not be found, or is an imported field. " + "Imported fields are not supported in conditional mutations.", + e.getFieldName().c_str()))); } - - // Retrieve document auto result = retrieveDocument(fieldVisitor.getFieldSet(), context); - // If document exists, match it with selection if (result.hasDocument()) { auto docPtr = result.getDocumentPtr(); if (_docSelectionUp->contains(*docPtr) != document::select::Result::True) { - return api::ReturnCode(api::ReturnCode::TEST_AND_SET_CONDITION_FAILED, - vespalib::make_string("Condition did not match document nodeIndex=%d bucket=%" PRIx64 " %s", - _env._nodeIndex, _cmd.getBucketId().getRawId(), - _cmd.hasBeenRemapped() ? "remapped" : "")); + return {result.getTimestamp(), Result::ConditionOutcome::IsNotMatch}; } - // Document matches - return api::ReturnCode(); - } else if (_missingDocumentImpliesMatch) { - return api::ReturnCode(); + return {result.getTimestamp(), Result::ConditionOutcome::IsMatch}; } + return {result.getTimestamp(), result.is_tombstone() ? Result::ConditionOutcome::IsTombstone + : Result::ConditionOutcome::DocNotFound}; +} - return api::ReturnCode(api::ReturnCode::TEST_AND_SET_CONDITION_FAILED, - vespalib::make_string("Document does not exist nodeIndex=%d bucket=%" PRIx64 " %s", - _env._nodeIndex, _cmd.getBucketId().getRawId(), - _cmd.hasBeenRemapped() ? "remapped" : "")); +api::ReturnCode +TestAndSetHelper::to_api_return_code(const Result& result) const { + switch (result.condition_outcome) { + case Result::ConditionOutcome::IsNotMatch: + return {api::ReturnCode::TEST_AND_SET_CONDITION_FAILED, + vespalib::make_string("Condition did not match document nodeIndex=%d bucket=%" PRIx64, + _env._nodeIndex, _bucket.getBucketId().getRawId())}; + case Result::ConditionOutcome::IsTombstone: + case Result::ConditionOutcome::DocNotFound: + if (!_missingDocumentImpliesMatch) { + return {api::ReturnCode::TEST_AND_SET_CONDITION_FAILED, + vespalib::make_string("Document does not exist nodeIndex=%d bucket=%" PRIx64, + _env._nodeIndex, _bucket.getBucketId().getRawId())}; + } + [[fallthrough]]; // as match + case Result::ConditionOutcome::IsMatch: + return {}; // OK + } + abort(); +} + +api::ReturnCode +TestAndSetHelper::retrieveAndMatch(spi::Context & context) { + auto result = fetch_and_match_raw(context); + return to_api_return_code(result); } } // storage diff --git a/storage/src/vespa/storage/persistence/testandsethelper.h b/storage/src/vespa/storage/persistence/testandsethelper.h index 82710e523c4..31b1cc79a54 100644 --- a/storage/src/vespa/storage/persistence/testandsethelper.h +++ b/storage/src/vespa/storage/persistence/testandsethelper.h @@ -25,9 +25,8 @@ class PersistenceUtil; class TestAndSetException : public std::runtime_error { api::ReturnCode _code; - public: - TestAndSetException(api::ReturnCode code) + explicit TestAndSetException(api::ReturnCode code) : std::runtime_error(code.getMessage()), _code(std::move(code)) {} @@ -36,11 +35,12 @@ public: }; class TestAndSetHelper { - const PersistenceUtil &_env; - const spi::PersistenceProvider &_spi; - const api::TestAndSetCommand &_cmd; + const PersistenceUtil& _env; + const spi::PersistenceProvider& _spi; + const documentapi::TestAndSetCondition& _condition; + const document::Bucket _bucket; const document::DocumentId _docId; - const document::DocumentType * _docTypePtr; + const document::DocumentType* _docTypePtr; std::unique_ptr<document::select::Node> _docSelectionUp; bool _missingDocumentImpliesMatch; @@ -50,10 +50,44 @@ class TestAndSetHelper { spi::GetResult retrieveDocument(const document::FieldSet & fieldSet, spi::Context & context); public: - TestAndSetHelper(const PersistenceUtil & env, const spi::PersistenceProvider & _spi, - const document::BucketIdFactory & bucketIdFactory, - const api::TestAndSetCommand & cmd, bool missingDocumentImpliesMatch = false); + struct Result { + enum class ConditionOutcome { + DocNotFound, + IsMatch, + IsNotMatch, + IsTombstone + }; + + api::Timestamp timestamp = 0; + ConditionOutcome condition_outcome = ConditionOutcome::IsNotMatch; + + [[nodiscard]] bool doc_not_found() const noexcept { + return condition_outcome == ConditionOutcome::DocNotFound; + } + [[nodiscard]] bool is_match() const noexcept { + return condition_outcome == ConditionOutcome::IsMatch; + } + [[nodiscard]] bool is_not_match() const noexcept { + return condition_outcome == ConditionOutcome::IsNotMatch; + } + [[nodiscard]] bool is_tombstone() const noexcept { + return condition_outcome == ConditionOutcome::IsTombstone; + } + }; + + TestAndSetHelper(const PersistenceUtil& env, + const spi::PersistenceProvider& _spi, + const document::BucketIdFactory& bucket_id_factory, + const documentapi::TestAndSetCondition& condition, + document::Bucket bucket, + document::DocumentId doc_id, + const document::DocumentType* doc_type_ptr, + bool missingDocumentImpliesMatch = false); ~TestAndSetHelper(); + + Result fetch_and_match_raw(spi::Context& context); + api::ReturnCode to_api_return_code(const Result& result) const; + api::ReturnCode retrieveAndMatch(spi::Context & context); }; diff --git a/storage/src/vespa/storageapi/message/persistence.cpp b/storage/src/vespa/storageapi/message/persistence.cpp index 41a53449b67..1b09639fd9b 100644 --- a/storage/src/vespa/storageapi/message/persistence.cpp +++ b/storage/src/vespa/storageapi/message/persistence.cpp @@ -222,7 +222,8 @@ GetReply::GetReply(const GetCommand& cmd, const DocumentSP& doc, Timestamp lastModified, bool had_consistent_replicas, - bool is_tombstone) + bool is_tombstone, + bool condition_matched) : BucketInfoReply(cmd), _docId(cmd.getDocumentId()), _fieldSet(cmd.getFieldSet()), @@ -230,7 +231,8 @@ GetReply::GetReply(const GetCommand& cmd, _beforeTimestamp(cmd.getBeforeTimestamp()), _lastModifiedTime(lastModified), _had_consistent_replicas(had_consistent_replicas), - _is_tombstone(is_tombstone) + _is_tombstone(is_tombstone), + _condition_matched(condition_matched) { } diff --git a/storage/src/vespa/storageapi/message/persistence.h b/storage/src/vespa/storageapi/message/persistence.h index d1709c46a6e..d010c295ca7 100644 --- a/storage/src/vespa/storageapi/message/persistence.h +++ b/storage/src/vespa/storageapi/message/persistence.h @@ -185,9 +185,10 @@ public: * timestamp. */ class GetCommand : public BucketInfoCommand { - document::DocumentId _docId; - Timestamp _beforeTimestamp; - vespalib::string _fieldSet; + document::DocumentId _docId; + Timestamp _beforeTimestamp; + vespalib::string _fieldSet; + TestAndSetCondition _condition; InternalReadConsistency _internal_read_consistency; public: GetCommand(const document::Bucket &bucket, const document::DocumentId&, @@ -198,6 +199,9 @@ public: Timestamp getBeforeTimestamp() const { return _beforeTimestamp; } const vespalib::string& getFieldSet() const { return _fieldSet; } void setFieldSet(vespalib::stringref fieldSet) { _fieldSet = fieldSet; } + [[nodiscard]] bool has_condition() const noexcept { return _condition.isPresent(); } + [[nodiscard]] const TestAndSetCondition& condition() const noexcept { return _condition; } + void set_condition(TestAndSetCondition cond) { _condition = std::move(cond); } InternalReadConsistency internal_read_consistency() const noexcept { return _internal_read_consistency; } @@ -229,12 +233,14 @@ class GetReply : public BucketInfoReply { Timestamp _lastModifiedTime; bool _had_consistent_replicas; bool _is_tombstone; + bool _condition_matched; public: explicit GetReply(const GetCommand& cmd, const DocumentSP& doc = DocumentSP(), Timestamp lastModified = 0, bool had_consistent_replicas = false, - bool is_tombstone = false); + bool is_tombstone = false, + bool condition_matched = false); ~GetReply() override; @@ -247,6 +253,7 @@ public: [[nodiscard]] bool had_consistent_replicas() const noexcept { return _had_consistent_replicas; } [[nodiscard]] bool is_tombstone() const noexcept { return _is_tombstone; } + [[nodiscard]] bool condition_matched() const noexcept { return _condition_matched; } bool wasFound() const { return (_doc.get() != nullptr); } void print(std::ostream& out, bool verbose, const std::string& indent) const override; diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java index 2a688ad078b..5126f7e0f43 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java @@ -526,23 +526,18 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { parameters = getProperty(request, TIMEOUT, timeoutMillisParser).map(clock.instant()::plusMillis) .map(parameters::withDeadline) .orElse(parameters); - for (String name : names) switch (name) { - case CLUSTER: - parameters = getProperty(request, CLUSTER).map(cluster -> resolveCluster(Optional.of(cluster), clusters).name()) - .map(parameters::withRoute) - .orElse(parameters); - break; - case FIELD_SET: - parameters = getProperty(request, FIELD_SET).map(parameters::withFieldSet) - .orElse(parameters); - break; - case ROUTE: - parameters = getProperty(request, ROUTE).map(parameters::withRoute) - .orElse(parameters); - break; - default: - throw new IllegalArgumentException("Unrecognized document operation parameter name '" + name + "'"); - } + for (String name : names) + parameters = switch (name) { + case CLUSTER -> + getProperty(request, CLUSTER) + .map(cluster -> resolveCluster(Optional.of(cluster), clusters).name()) + .map(parameters::withRoute) + .orElse(parameters); + case FIELD_SET -> getProperty(request, FIELD_SET).map(parameters::withFieldSet).orElse(parameters); + case ROUTE -> getProperty(request, ROUTE).map(parameters::withRoute).orElse(parameters); + default -> + throw new IllegalArgumentException("Unrecognized document operation parameter name '" + name + "'"); + }; return parameters; } @@ -630,10 +625,6 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { private boolean first = true; private ContentChannel channel; - private JsonResponse(ResponseHandler handler) throws IOException { - this(handler, null); - } - private JsonResponse(ResponseHandler handler, HttpRequest request) throws IOException { this.handler = handler; this.request = request; @@ -642,11 +633,6 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } /** Creates a new JsonResponse with path and id fields written. */ - static JsonResponse create(DocumentPath path, ResponseHandler handler) throws IOException { - return create(path, handler, null); - } - - /** Creates a new JsonResponse with path and id fields written. */ static JsonResponse create(DocumentPath path, ResponseHandler handler, HttpRequest request) throws IOException { JsonResponse response = new JsonResponse(handler, request); response.writePathId(path.rawPath()); @@ -749,23 +735,17 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } private boolean tensorShortForm() { - if (request != null && - request.parameters().containsKey("format.tensors") && - ( request.parameters().get("format.tensors").contains("long") - || request.parameters().get("format.tensors").contains("long-value"))) { - return false; - } - return true; // default + return request == null || + !request.parameters().containsKey("format.tensors") || + (!request.parameters().get("format.tensors").contains("long") + && !request.parameters().get("format.tensors").contains("long-value"));// default } private boolean tensorDirectValues() { - if (request != null && - request.parameters().containsKey("format.tensors") && - ( request.parameters().get("format.tensors").contains("short-value") - || request.parameters().get("format.tensors").contains("long-value"))) { - return true; - } - return false; // TODO: Flip default on Vespa 9 + return request != null && + request.parameters().containsKey("format.tensors") && + (request.parameters().get("format.tensors").contains("short-value") + || request.parameters().get("format.tensors").contains("long-value"));// TODO: Flip default on Vespa 9 } synchronized void writeSingleDocument(Document document) throws IOException { @@ -1168,9 +1148,8 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { // ------------------------------------------------- Visits ------------------------------------------------ private VisitorParameters parseGetParameters(HttpRequest request, DocumentPath path, boolean streamed) { - int wantedDocumentCount = Math.min(streamed ? Integer.MAX_VALUE : 1 << 10, - getProperty(request, WANTED_DOCUMENT_COUNT, integerParser) - .orElse(streamed ? Integer.MAX_VALUE : 1)); + int wantedDocumentCount = getProperty(request, WANTED_DOCUMENT_COUNT, integerParser) + .orElse(streamed ? Integer.MAX_VALUE : 1); if (wantedDocumentCount <= 0) throw new IllegalArgumentException("wantedDocumentCount must be positive"); @@ -1546,11 +1525,11 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { private static Map<String, StorageCluster> parseClusters(ClusterListConfig clusters, AllClustersBucketSpacesConfig buckets) { return clusters.storage().stream() - .collect(toUnmodifiableMap(storage -> storage.name(), + .collect(toUnmodifiableMap(ClusterListConfig.Storage::name, storage -> new StorageCluster(storage.name(), buckets.cluster(storage.name()) .documentType().entrySet().stream() - .collect(toMap(entry -> entry.getKey(), + .collect(toMap(Map.Entry::getKey, entry -> entry.getValue().bucketSpace()))))); } diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java index 851a0949266..7696fd2196c 100644 --- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java +++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java @@ -217,7 +217,7 @@ public class DocumentV1ApiTest { access.expect(parameters -> { assertEquals("content", parameters.getRoute().toString()); assertEquals("default", parameters.getBucketSpace()); - assertEquals(1024, parameters.getMaxTotalHits()); + assertEquals(1025, parameters.getMaxTotalHits()); assertEquals(100, ((StaticThrottlePolicy) parameters.getThrottlePolicy()).getMaxPendingCount()); assertEquals("[id]", parameters.getFieldSet()); assertEquals("(all the things)", parameters.getDocumentSelection()); |