diff options
31 files changed, 318 insertions, 54 deletions
diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/RoutableFactories80.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/RoutableFactories80.java index 778eaeda5f0..fb78e291c7c 100644 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/RoutableFactories80.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/RoutableFactories80.java @@ -387,6 +387,8 @@ abstract class RoutableFactories80 { if (apiMsg.getCondition().isPresent()) { builder.setCondition(toProtoTasCondition(apiMsg.getCondition())); } + builder.setCreateIfMissing(apiMsg.createIfMissing() ? DocapiFeed.UpdateDocumentRequest.CreateIfMissing.TRUE + : DocapiFeed.UpdateDocumentRequest.CreateIfMissing.FALSE); return builder.build(); }) .decoderWithRepo(DocapiFeed.UpdateDocumentRequest.parser(), (protoMsg, repo) -> { @@ -396,6 +398,8 @@ abstract class RoutableFactories80 { if (protoMsg.hasCondition()) { msg.setCondition(fromProtoTasCondition(protoMsg.getCondition())); } + // We ignore the createIfMissing field here since it can always be fetched eagerly + // from the DocumentUpdate instance itself. return msg; }) .build(); diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/UpdateDocumentMessage.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/UpdateDocumentMessage.java index 3fb14664628..b8609cd42b8 100755 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/UpdateDocumentMessage.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/UpdateDocumentMessage.java @@ -166,4 +166,10 @@ public class UpdateDocumentMessage extends TestAndSetMessage { public void setCondition(TestAndSetCondition condition) { this.update.setCondition(condition); } + + boolean createIfMissing() { + deserialize(); + return update.getCreateIfNonExistent(); + } + } diff --git a/documentapi/src/protobuf/docapi_feed.proto b/documentapi/src/protobuf/docapi_feed.proto index 8d15fd9a536..702695ef6d8 100644 --- a/documentapi/src/protobuf/docapi_feed.proto +++ b/documentapi/src/protobuf/docapi_feed.proto @@ -39,11 +39,18 @@ message PutDocumentResponse { } message UpdateDocumentRequest { + enum CreateIfMissing { + UNSPECIFIED = 0; // Legacy fallback: must deserialize `update` to find flag value + TRUE = 1; + FALSE = 2; + } + // Note: update contains embedded document ID DocumentUpdate update = 1; TestAndSetCondition condition = 2; uint64 expected_old_timestamp = 3; uint64 force_assign_timestamp = 4; + CreateIfMissing create_if_missing = 5; } message UpdateDocumentResponse { diff --git a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages60TestCase.java b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages60TestCase.java index 42f200a0b6b..803b225bb0d 100644 --- a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages60TestCase.java +++ b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages60TestCase.java @@ -504,6 +504,7 @@ public class Messages60TestCase extends MessagesTestBase { assertEquals(msg.getNewTimestamp(), deserializedMsg.getNewTimestamp()); assertEquals(msg.getOldTimestamp(), deserializedMsg.getOldTimestamp()); assertEquals(msg.getCondition().getSelection(), deserializedMsg.getCondition().getSelection()); + assertFalse(msg.createIfMissing()); } } } diff --git a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages80TestCase.java b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages80TestCase.java index 943d9fddb26..d6a837229a9 100644 --- a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages80TestCase.java +++ b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/Messages80TestCase.java @@ -176,8 +176,52 @@ public class Messages80TestCase extends MessagesTestBase { } class UpdateDocumentMessageTest implements RunnableTest { - @Override - public void run() { + + UpdateDocumentMessage makeUpdateWithCreateIfMissing(boolean createIfMissing) { + var docType = protocol.getDocumentTypeManager().getDocumentType("testdoc"); + var update = new DocumentUpdate(docType, new DocumentId("id:ns:testdoc::")); + update.addFieldPathUpdate(new RemoveFieldPathUpdate(docType, "intfield", "testdoc.intfield > 0")); + update.setCreateIfNonExistent(createIfMissing); + + var msg = new UpdateDocumentMessage(update); + msg.setNewTimestamp(777); + msg.setOldTimestamp(666); + msg.setCondition(new TestAndSetCondition(CONDITION_STRING)); + return msg; + } + + void testLegacyCreateIfMissingFlagCanBeDeserializedFromDocumentUpdate() { + // Legacy binary files were created _prior_ to the createIfMissing flag being + // written as part of the serialization process. + forEachLanguage((lang) -> { + var msg = (UpdateDocumentMessage) deserialize( + "UpdateDocumentMessage-legacy-no-create-if-missing", + DocumentProtocol.MESSAGE_UPDATEDOCUMENT, lang); + assertFalse(msg.createIfMissing()); + + msg = (UpdateDocumentMessage) deserialize( + "UpdateDocumentMessage-legacy-with-create-if-missing", + DocumentProtocol.MESSAGE_UPDATEDOCUMENT, lang); + assertTrue(msg.createIfMissing()); + }); + } + + void checkDeserialization(Language lang, String name, boolean expectedCreate) { + var msg = (UpdateDocumentMessage) deserialize(name, DocumentProtocol.MESSAGE_UPDATEDOCUMENT, lang); + assertEquals(expectedCreate, msg.createIfMissing()); + }; + + void testCreateIfMissingFlagIsPropagated() { + serialize("UpdateDocumentMessage-no-create-if-missing", makeUpdateWithCreateIfMissing(false)); + serialize("UpdateDocumentMessage-with-create-if-missing", makeUpdateWithCreateIfMissing(true)); + + forEachLanguage((lang) -> { + checkDeserialization(lang, "UpdateDocumentMessage-no-create-if-missing", false); + checkDeserialization(lang, "UpdateDocumentMessage-with-create-if-missing", true); + }); + } + + void testAllUpdateFieldsArePropagated() { var docType = protocol.getDocumentTypeManager().getDocumentType("testdoc"); var update = new DocumentUpdate(docType, new DocumentId("id:ns:testdoc::")); update.addFieldPathUpdate(new RemoveFieldPathUpdate(docType, "intfield", "testdoc.intfield > 0")); @@ -197,6 +241,13 @@ public class Messages80TestCase extends MessagesTestBase { assertEquals(msg.getCondition().getSelection(), deserializedMsg.getCondition().getSelection()); }); } + + @Override + public void run() { + testAllUpdateFieldsArePropagated(); + testLegacyCreateIfMissingFlagCanBeDeserializedFromDocumentUpdate(); + testCreateIfMissingFlagIsPropagated(); + } } class UpdateDocumentReplyTest implements RunnableTest { diff --git a/documentapi/src/tests/messages/messages60test.cpp b/documentapi/src/tests/messages/messages60test.cpp index 2af523ce8e9..861189a2d33 100644 --- a/documentapi/src/tests/messages/messages60test.cpp +++ b/documentapi/src/tests/messages/messages60test.cpp @@ -406,7 +406,7 @@ TEST_F(Messages60Test, testUpdateDocumentMessage) { msg.setNewTimestamp(777u); msg.setCondition(TestAndSetCondition("There's just one condition")); - EXPECT_EQ(sizeof(TestAndSetMessage) + 32, sizeof(UpdateDocumentMessage)); + EXPECT_EQ(sizeof(TestAndSetMessage) + 40, sizeof(UpdateDocumentMessage)); EXPECT_EQ(MESSAGE_BASE_LENGTH + 93u + serializedLength(msg.getCondition().getSelection()), serialize("UpdateDocumentMessage", msg)); for (uint32_t lang = 0; lang < NUM_LANGUAGES; ++lang) { diff --git a/documentapi/src/tests/messages/messages80test.cpp b/documentapi/src/tests/messages/messages80test.cpp index c61b1575dac..093e44ed6b1 100644 --- a/documentapi/src/tests/messages/messages80test.cpp +++ b/documentapi/src/tests/messages/messages80test.cpp @@ -29,7 +29,7 @@ TEST(MessagesTest, concrete_types_have_expected_sizes) { EXPECT_EQ(sizeof(PutDocumentMessage), sizeof(TestAndSetMessage) + 32); EXPECT_EQ(sizeof(WriteDocumentReply), 112u); EXPECT_EQ(sizeof(UpdateDocumentReply), 120u); - EXPECT_EQ(sizeof(UpdateDocumentMessage), sizeof(TestAndSetMessage) + 32); + EXPECT_EQ(sizeof(UpdateDocumentMessage), sizeof(TestAndSetMessage) + 40); EXPECT_EQ(sizeof(RemoveDocumentMessage), sizeof(TestAndSetMessage) + 104); EXPECT_EQ(sizeof(RemoveDocumentReply), 120u); } @@ -42,6 +42,14 @@ struct Messages80Test : MessageFixture { } void try_visitor_reply(const std::string& filename, uint32_t type); + + void check_update_create_flag(uint32_t lang, const std::string& name, bool expected_create, bool expected_cached) { + auto obj = deserialize(name, DocumentProtocol::MESSAGE_UPDATEDOCUMENT, lang); + ASSERT_TRUE(obj); + auto& msg = dynamic_cast<UpdateDocumentMessage&>(*obj); + EXPECT_EQ(msg.has_cached_create_if_missing(), expected_cached); + EXPECT_EQ(msg.create_if_missing(), expected_create); + }; }; namespace { @@ -180,6 +188,48 @@ TEST_F(Messages80Test, update_document_message) { } } +TEST_F(Messages80Test, update_create_if_missing_flag_can_be_read_from_legacy_update_propagation) { + // Legacy binary files were created _prior_ to the create_if_missing flag being + // written as part of the serialization process. + for (auto lang : languages()) { + check_update_create_flag(lang, "UpdateDocumentMessage-legacy-no-create-if-missing", false, false); + check_update_create_flag(lang, "UpdateDocumentMessage-legacy-with-create-if-missing", true, false); + } +} + +TEST_F(Messages80Test, update_create_if_missing_flag_is_propagated) { + const DocumentTypeRepo& repo = type_repo(); + const document::DocumentType& docType = *repo.getDocumentType("testdoc"); + + auto make_update_msg = [&](bool create_if_missing, bool cache_flag) { + auto doc_update = std::make_shared<document::DocumentUpdate>(repo, docType, document::DocumentId("id:ns:testdoc::")); + doc_update->addFieldPathUpdate(std::make_unique<document::RemoveFieldPathUpdate>("intfield", "testdoc.intfield > 0")); + doc_update->setCreateIfNonExistent(create_if_missing); + auto msg = std::make_shared<UpdateDocumentMessage>(std::move(doc_update)); + msg->setOldTimestamp(666u); + msg->setNewTimestamp(777u); + msg->setCondition(TestAndSetCondition("There's just one condition")); + if (cache_flag) { + msg->set_cached_create_if_missing(create_if_missing); + } + return msg; + }; + + serialize("UpdateDocumentMessage-no-create-if-missing", *make_update_msg(false, true)); + serialize("UpdateDocumentMessage-with-create-if-missing", *make_update_msg(true, true)); + + for (auto lang : languages()) { + check_update_create_flag(lang, "UpdateDocumentMessage-no-create-if-missing", false, true); + check_update_create_flag(lang, "UpdateDocumentMessage-with-create-if-missing", true, true); + } + // The Java protocol implementation always serializes with a cached create-flag, + // but the C++ side does it conditionally. So these files are only checked for C++. + serialize("UpdateDocumentMessage-no-create-if-missing-uncached", *make_update_msg(false, false)); + serialize("UpdateDocumentMessage-with-create-if-missing-uncached", *make_update_msg(true, false)); + check_update_create_flag(LANG_CPP, "UpdateDocumentMessage-no-create-if-missing-uncached", false, false); + check_update_create_flag(LANG_CPP, "UpdateDocumentMessage-with-create-if-missing-uncached", true, false); +} + TEST_F(Messages80Test, update_document_reply) { UpdateDocumentReply reply; reply.setWasFound(true); diff --git a/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.cpp b/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.cpp index 3c0ffa33060..d416e587ed6 100644 --- a/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.cpp +++ b/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.cpp @@ -5,6 +5,7 @@ #include <vespa/documentapi/messagebus/documentprotocol.h> #include <vespa/document/update/documentupdate.h> #include <vespa/vespalib/util/exceptions.h> +#include <cassert> namespace documentapi { @@ -12,14 +13,16 @@ UpdateDocumentMessage::UpdateDocumentMessage() : TestAndSetMessage(), _documentUpdate(), _oldTime(0), - _newTime(0) + _newTime(0), + _create_if_missing() {} UpdateDocumentMessage::UpdateDocumentMessage(document::DocumentUpdate::SP documentUpdate) : TestAndSetMessage(), _documentUpdate(), _oldTime(0), - _newTime(0) + _newTime(0), + _create_if_missing() { setDocumentUpdate(std::move(documentUpdate)); } @@ -59,4 +62,14 @@ UpdateDocumentMessage::setDocumentUpdate(document::DocumentUpdate::SP documentUp _documentUpdate = std::move(documentUpdate); } +bool +UpdateDocumentMessage::create_if_missing() const +{ + if (_create_if_missing.has_value()) { + return *_create_if_missing; + } + assert(_documentUpdate); + return _documentUpdate->getCreateIfNonExistent(); +} + } diff --git a/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.h b/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.h index 55aa0bf8ae4..e4a528dacdd 100644 --- a/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.h +++ b/documentapi/src/vespa/documentapi/messagebus/messages/updatedocumentmessage.h @@ -2,6 +2,7 @@ #pragma once #include "testandsetmessage.h" +#include <optional> namespace document { class DocumentUpdate; } @@ -10,9 +11,10 @@ namespace documentapi { class UpdateDocumentMessage : public TestAndSetMessage { private: using DocumentUpdateSP = std::shared_ptr<document::DocumentUpdate>; - DocumentUpdateSP _documentUpdate; - uint64_t _oldTime; - uint64_t _newTime; + DocumentUpdateSP _documentUpdate; + uint64_t _oldTime; + uint64_t _newTime; + std::optional<bool> _create_if_missing; protected: DocumentReply::UP doCreateReply() const override; @@ -28,21 +30,21 @@ public: * Constructs a new document message for deserialization. */ UpdateDocumentMessage(); - ~UpdateDocumentMessage(); + ~UpdateDocumentMessage() override; /** * Constructs a new document update message. * * @param documentUpdate The document update to perform. */ - UpdateDocumentMessage(DocumentUpdateSP documentUpdate); + explicit UpdateDocumentMessage(DocumentUpdateSP documentUpdate); /** * Returns the document update to perform. * * @return The update. */ - DocumentUpdateSP stealDocumentUpdate() const { return std::move(_documentUpdate); } + [[nodiscard]] DocumentUpdateSP stealDocumentUpdate() const { return std::move(_documentUpdate); } const document::DocumentUpdate & getDocumentUpdate() const { return *_documentUpdate; } /** * Sets the document update to perform. @@ -56,21 +58,21 @@ public: * * @return The document timestamp. */ - uint64_t getOldTimestamp() const { return _oldTime; } + [[nodiscard]] uint64_t getOldTimestamp() const noexcept { return _oldTime; } /** * Sets the timestamp required for this update to be applied. * * @param time The timestamp to set. */ - void setOldTimestamp(uint64_t time) { _oldTime = time; } + void setOldTimestamp(uint64_t time) noexcept { _oldTime = time; } /** * Returns the timestamp to assign to the updated document. * * @return The document timestamp. */ - uint64_t getNewTimestamp() const { return _newTime; } + [[nodiscard]] uint64_t getNewTimestamp() const noexcept { return _newTime; } /** * Sets the timestamp to assign to the updated document. @@ -79,6 +81,18 @@ public: */ void setNewTimestamp(uint64_t time) { _newTime = time; } + void set_cached_create_if_missing(bool create) noexcept { + _create_if_missing = create; + } + + [[nodiscard]] bool has_cached_create_if_missing() const noexcept { + return _create_if_missing.has_value(); + } + // Note: iff has_cached_create_if_missing() == false, this will trigger a deserialization of the + // underlying DocumentUpdate instance, which might throw an exception on deserialization failure. + // Otherwise, this is noexcept. + [[nodiscard]] bool create_if_missing() const; + bool hasSequenceId() const override; uint64_t getSequenceId() const override; uint32_t getType() const override; diff --git a/documentapi/src/vespa/documentapi/messagebus/routable_factories_8.cpp b/documentapi/src/vespa/documentapi/messagebus/routable_factories_8.cpp index f9782e1abd9..9ef54932f68 100644 --- a/documentapi/src/vespa/documentapi/messagebus/routable_factories_8.cpp +++ b/documentapi/src/vespa/documentapi/messagebus/routable_factories_8.cpp @@ -312,6 +312,10 @@ std::shared_ptr<IRoutableFactory> RoutableFactories80::update_document_message_f } dest.set_expected_old_timestamp(src.getOldTimestamp()); dest.set_force_assign_timestamp(src.getNewTimestamp()); + if (src.has_cached_create_if_missing()) { + dest.set_create_if_missing(src.create_if_missing() ? protobuf::UpdateDocumentRequest_CreateIfMissing_TRUE + : protobuf::UpdateDocumentRequest_CreateIfMissing_FALSE); + } }, [type_repo = std::move(repo)](const protobuf::UpdateDocumentRequest& src) { auto msg = std::make_unique<UpdateDocumentMessage>(); @@ -321,6 +325,9 @@ std::shared_ptr<IRoutableFactory> RoutableFactories80::update_document_message_f } msg->setOldTimestamp(src.expected_old_timestamp()); msg->setNewTimestamp(src.force_assign_timestamp()); + if (src.create_if_missing() != protobuf::UpdateDocumentRequest_CreateIfMissing_UNSPECIFIED) { + msg->set_cached_create_if_missing(src.create_if_missing() == protobuf::UpdateDocumentRequest_CreateIfMissing_TRUE); + } return msg; } ); diff --git a/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-legacy-no-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-legacy-no-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..f1ceef0e51a --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-legacy-no-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-legacy-with-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-legacy-with-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..840bd693670 --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-legacy-with-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-no-create-if-missing-uncached.dat b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-no-create-if-missing-uncached.dat Binary files differnew file mode 100644 index 00000000000..f1ceef0e51a --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-no-create-if-missing-uncached.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-no-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-no-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..fc42a504f8b --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-no-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-with-create-if-missing-uncached.dat b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-with-create-if-missing-uncached.dat Binary files differnew file mode 100644 index 00000000000..840bd693670 --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-with-create-if-missing-uncached.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-with-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-with-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..ea4852b2e7f --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-cpp-UpdateDocumentMessage-with-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-legacy-no-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-legacy-no-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..f1ceef0e51a --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-legacy-no-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-legacy-with-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-legacy-with-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..840bd693670 --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-legacy-with-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-no-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-no-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..fc42a504f8b --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-no-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-with-create-if-missing.dat b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-with-create-if-missing.dat Binary files differnew file mode 100644 index 00000000000..ea4852b2e7f --- /dev/null +++ b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage-with-create-if-missing.dat diff --git a/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage.dat b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage.dat Binary files differindex f1ceef0e51a..fc42a504f8b 100644 --- a/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage.dat +++ b/documentapi/test/crosslanguagefiles/8.310-java-UpdateDocumentMessage.dat diff --git a/storage/src/tests/distributor/updateoperationtest.cpp b/storage/src/tests/distributor/updateoperationtest.cpp index 31ebbe19cbb..e00ce249298 100644 --- a/storage/src/tests/distributor/updateoperationtest.cpp +++ b/storage/src/tests/distributor/updateoperationtest.cpp @@ -49,13 +49,13 @@ struct UpdateOperationTest : Test, DistributorStripeTestUtil { const api::ReturnCode& result = api::ReturnCode()); std::shared_ptr<UpdateOperation> - sendUpdate(const std::string& bucketState, bool create_if_missing = false); + sendUpdate(const std::string& bucketState, bool create_if_missing = false, bool cache_create_flag = false); document::BucketId _bId; }; std::shared_ptr<UpdateOperation> -UpdateOperationTest::sendUpdate(const std::string& bucketState, bool create_if_missing) +UpdateOperationTest::sendUpdate(const std::string& bucketState, bool create_if_missing, bool cache_create_flag) { auto update = std::make_shared<document::DocumentUpdate>( *_repo, *_html_type, @@ -67,6 +67,9 @@ UpdateOperationTest::sendUpdate(const std::string& bucketState, bool create_if_m addNodesToBucketDB(_bId, bucketState); auto msg = std::make_shared<api::UpdateCommand>(makeDocumentBucket(document::BucketId(0)), update, 100); + if (cache_create_flag) { + msg->set_cached_create_if_missing(create_if_missing); + } return std::make_shared<UpdateOperation>( node_context(), operation_context(), getDistributorBucketSpace(), msg, std::vector<BucketDatabase::Entry>(), @@ -271,4 +274,20 @@ TEST_F(UpdateOperationTest, cancelled_nodes_are_not_updated_in_db) { dumpBucket(_bId)); } +TEST_F(UpdateOperationTest, cached_create_if_missing_is_propagated_to_fanout_requests) { + setup_stripe(1, 1, "distributor:1 storage:1"); + for (bool cache_flag : {false, true}) { + for (bool create_if_missing : {false, true}) { + std::shared_ptr<UpdateOperation> cb(sendUpdate("0=1/2/3", create_if_missing, cache_flag)); + DistributorMessageSenderStub sender; + cb->start(sender); + + ASSERT_EQ("Update => 0", sender.getCommands(true)); + auto& cmd = dynamic_cast<api::UpdateCommand&>(*sender.command(0)); + EXPECT_EQ(cmd.has_cached_create_if_missing(), cache_flag); + EXPECT_EQ(cmd.create_if_missing(), create_if_missing); + } + } +} + } diff --git a/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp b/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp index 698d8dee573..231f41ffd21 100644 --- a/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp +++ b/storage/src/tests/storageapi/mbusprot/storageprotocoltest.cpp @@ -262,7 +262,6 @@ TEST_P(StorageProtocolTest, response_metadata_is_propagated) { TEST_P(StorageProtocolTest, update) { auto update = std::make_shared<document::DocumentUpdate>(_docMan.getTypeRepo(), *_testDoc->getDataType(), _testDoc->getId()); update->addUpdate(FieldUpdate(_testDoc->getField("headerval")).addUpdate(std::make_unique<AssignValueUpdate>(std::make_unique<IntFieldValue>(17)))); - update->addFieldPathUpdate(std::make_unique<RemoveFieldPathUpdate>("headerval", "testdoctype1.headerval > 0")); auto cmd = std::make_shared<UpdateCommand>(_bucket, update, 14); @@ -284,6 +283,37 @@ TEST_P(StorageProtocolTest, update) { EXPECT_NO_FATAL_FAILURE(assert_bucket_info_reply_fields_propagated(*reply2)); } +TEST_P(StorageProtocolTest, update_request_create_if_missing_flag_is_propagated) { + auto make_update_cmd = [&](bool create_if_missing, bool cached) { + auto update = std::make_shared<document::DocumentUpdate>( + _docMan.getTypeRepo(), *_testDoc->getDataType(), _testDoc->getId()); + update->addUpdate(FieldUpdate(_testDoc->getField("headerval")).addUpdate( + std::make_unique<AssignValueUpdate>(std::make_unique<IntFieldValue>(17)))); + update->addFieldPathUpdate(std::make_unique<RemoveFieldPathUpdate>("headerval", "testdoctype1.headerval > 0")); + update->setCreateIfNonExistent(create_if_missing); + auto cmd = std::make_shared<UpdateCommand>(_bucket, update, 14); + if (cached) { + cmd->set_cached_create_if_missing(create_if_missing); + } + return cmd; + }; + + auto check_flag_propagation = [&](bool create_if_missing, bool cached) { + auto cmd = make_update_cmd(create_if_missing, cached); + EXPECT_EQ(cmd->has_cached_create_if_missing(), cached); + EXPECT_EQ(cmd->create_if_missing(), create_if_missing); + + auto cmd2 = copyCommand(cmd); + EXPECT_EQ(cmd2->has_cached_create_if_missing(), cached); + EXPECT_EQ(cmd2->create_if_missing(), create_if_missing); + }; + + check_flag_propagation(false, false); + check_flag_propagation(true, false); + check_flag_propagation(false, true); + check_flag_propagation(true, true); +} + TEST_P(StorageProtocolTest, get) { auto cmd = std::make_shared<GetCommand>(_bucket, _testDocId, "foo,bar,vekterli", 123); auto cmd2 = copyCommand(cmd); @@ -880,7 +910,7 @@ TEST_P(StorageProtocolTest, track_memory_footprint_for_some_messages) { EXPECT_EQ(sizeof(BucketInfoCommand), sizeof(BucketCommand)); EXPECT_EQ(sizeof(TestAndSetCommand), sizeof(BucketInfoCommand) + sizeof(vespalib::string)); EXPECT_EQ(sizeof(PutCommand), sizeof(TestAndSetCommand) + 40); - EXPECT_EQ(sizeof(UpdateCommand), sizeof(TestAndSetCommand) + 32); + EXPECT_EQ(sizeof(UpdateCommand), sizeof(TestAndSetCommand) + 40); EXPECT_EQ(sizeof(RemoveCommand), sizeof(TestAndSetCommand) + 112); EXPECT_EQ(sizeof(GetCommand), sizeof(BucketInfoCommand) + sizeof(TestAndSetCondition) + 184); } diff --git a/storage/src/tests/storageserver/documentapiconvertertest.cpp b/storage/src/tests/storageserver/documentapiconvertertest.cpp index eb4789b25d4..1eb6bf5dd9a 100644 --- a/storage/src/tests/storageserver/documentapiconvertertest.cpp +++ b/storage/src/tests/storageserver/documentapiconvertertest.cpp @@ -159,28 +159,46 @@ TEST_F(DocumentApiConverterTest, forwarded_put) { } TEST_F(DocumentApiConverterTest, update) { - auto update = std::make_shared<document::DocumentUpdate>(*_repo, _html_type, defaultDocId); - documentapi::UpdateDocumentMessage updateMsg(update); - updateMsg.setOldTimestamp(1234); - updateMsg.setNewTimestamp(5678); - updateMsg.setCondition(my_condition); - - auto updateCmd = toStorageAPI<api::UpdateCommand>(updateMsg); - EXPECT_EQ(defaultBucket, updateCmd->getBucket()); - ASSERT_EQ(update.get(), updateCmd->getUpdate().get()); - EXPECT_EQ(api::Timestamp(1234), updateCmd->getOldTimestamp()); - EXPECT_EQ(api::Timestamp(5678), updateCmd->getTimestamp()); - EXPECT_EQ(my_condition, updateCmd->getCondition()); - - auto mbusReply = updateMsg.createReply(); - ASSERT_TRUE(mbusReply.get()); - toStorageAPI<api::UpdateReply>(*mbusReply, *updateCmd); - - auto mbusUpdate = toDocumentAPI<documentapi::UpdateDocumentMessage>(*updateCmd); - ASSERT_EQ((&mbusUpdate->getDocumentUpdate()), update.get()); - EXPECT_EQ(api::Timestamp(1234), mbusUpdate->getOldTimestamp()); - EXPECT_EQ(api::Timestamp(5678), mbusUpdate->getNewTimestamp()); - EXPECT_EQ(my_condition, mbusUpdate->getCondition()); + auto do_test_update = [&](bool create_if_missing) { + auto update = std::make_shared<document::DocumentUpdate>(*_repo, _html_type, defaultDocId); + update->setCreateIfNonExistent(create_if_missing); + documentapi::UpdateDocumentMessage updateMsg(update); + updateMsg.setOldTimestamp(1234); + updateMsg.setNewTimestamp(5678); + updateMsg.setCondition(my_condition); + EXPECT_FALSE(updateMsg.has_cached_create_if_missing()); + EXPECT_EQ(updateMsg.create_if_missing(), create_if_missing); + + auto updateCmd = toStorageAPI<api::UpdateCommand>(updateMsg); + EXPECT_EQ(defaultBucket, updateCmd->getBucket()); + ASSERT_EQ(update.get(), updateCmd->getUpdate().get()); + EXPECT_EQ(api::Timestamp(1234), updateCmd->getOldTimestamp()); + EXPECT_EQ(api::Timestamp(5678), updateCmd->getTimestamp()); + EXPECT_EQ(my_condition, updateCmd->getCondition()); + EXPECT_FALSE(updateCmd->has_cached_create_if_missing()); + EXPECT_EQ(updateCmd->create_if_missing(), create_if_missing); + + auto mbusReply = updateMsg.createReply(); + ASSERT_TRUE(mbusReply.get()); + toStorageAPI<api::UpdateReply>(*mbusReply, *updateCmd); + + auto mbusUpdate = toDocumentAPI<documentapi::UpdateDocumentMessage>(*updateCmd); + ASSERT_EQ((&mbusUpdate->getDocumentUpdate()), update.get()); + EXPECT_EQ(api::Timestamp(1234), mbusUpdate->getOldTimestamp()); + EXPECT_EQ(api::Timestamp(5678), mbusUpdate->getNewTimestamp()); + EXPECT_EQ(my_condition, mbusUpdate->getCondition()); + EXPECT_EQ(mbusUpdate->create_if_missing(), create_if_missing); + + // Cached value of create_if_missing should override underlying update's value + updateCmd->set_cached_create_if_missing(!create_if_missing); + EXPECT_TRUE(updateCmd->has_cached_create_if_missing()); + EXPECT_EQ(updateCmd->create_if_missing(), !create_if_missing); + mbusUpdate = toDocumentAPI<documentapi::UpdateDocumentMessage>(*updateCmd); + EXPECT_TRUE(mbusUpdate->has_cached_create_if_missing()); + EXPECT_EQ(mbusUpdate->create_if_missing(), !create_if_missing); + }; + do_test_update(false); + do_test_update(true); } TEST_F(DocumentApiConverterTest, remove) { diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp index 84e9ab71bcb..849746416d6 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp @@ -668,7 +668,7 @@ TwoPhaseUpdateOperation::applyUpdateToDocument(document::Document& doc) const bool TwoPhaseUpdateOperation::shouldCreateIfNonExistent() const { - return _updateCmd->getUpdate()->getCreateIfNonExistent(); + return _updateCmd->create_if_missing(); } bool diff --git a/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp index 7b6833cc299..2b47d53363f 100644 --- a/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp @@ -29,7 +29,7 @@ UpdateOperation::UpdateOperation(const DistributorNodeContext& node_ctx, _msg(msg), _entries(std::move(entries)), _new_timestamp(_msg->getTimestamp()), - _is_auto_create_update(_msg->getUpdate()->getCreateIfNonExistent()), + _is_auto_create_update(_msg->create_if_missing()), _node_ctx(node_ctx), _op_ctx(op_ctx), _bucketSpace(bucketSpace), @@ -112,6 +112,9 @@ UpdateOperation::onStart(DistributorStripeMessageSender& sender) copyMessageSettings(*_msg, *command); command->setOldTimestamp(_msg->getOldTimestamp()); command->setCondition(_msg->getCondition()); + if (_msg->has_cached_create_if_missing()) { + command->set_cached_create_if_missing(_msg->create_if_missing()); + } messages.emplace_back(std::move(command), node); } diff --git a/storage/src/vespa/storage/storageserver/documentapiconverter.cpp b/storage/src/vespa/storage/storageserver/documentapiconverter.cpp index ca46e87285b..5b8052a05f8 100644 --- a/storage/src/vespa/storage/storageserver/documentapiconverter.cpp +++ b/storage/src/vespa/storage/storageserver/documentapiconverter.cpp @@ -54,6 +54,9 @@ DocumentApiConverter::toStorageAPI(documentapi::DocumentMessage& fromMsg) auto to = std::make_unique<api::UpdateCommand>(bucket, from.stealDocumentUpdate(), from.getNewTimestamp()); to->setOldTimestamp(from.getOldTimestamp()); to->setCondition(from.getCondition()); + if (from.has_cached_create_if_missing()) { + to->set_cached_create_if_missing(from.create_if_missing()); + } toMsg = std::move(to); break; } @@ -217,6 +220,9 @@ DocumentApiConverter::toDocumentAPI(api::StorageCommand& fromMsg) to->setOldTimestamp(from.getOldTimestamp()); to->setNewTimestamp(from.getTimestamp()); to->setCondition(from.getCondition()); + if (from.has_cached_create_if_missing()) { + to->set_cached_create_if_missing(from.create_if_missing()); + } toMsg = std::move(to); break; } diff --git a/storage/src/vespa/storageapi/mbusprot/protobuf/feed.proto b/storage/src/vespa/storageapi/mbusprot/protobuf/feed.proto index 55d516a017b..403752b0c84 100644 --- a/storage/src/vespa/storageapi/mbusprot/protobuf/feed.proto +++ b/storage/src/vespa/storageapi/mbusprot/protobuf/feed.proto @@ -31,11 +31,18 @@ message Update { } message UpdateRequest { - Bucket bucket = 1; - Update update = 2; - uint64 new_timestamp = 3; - uint64 expected_old_timestamp = 4; // If zero; no expectation. - TestAndSetCondition condition = 5; + enum CreateIfMissing { + UNSPECIFIED = 0; // Legacy fallback: must deserialize `update` to find flag value + TRUE = 1; + FALSE = 2; + } + + Bucket bucket = 1; + Update update = 2; + uint64 new_timestamp = 3; + uint64 expected_old_timestamp = 4; // If zero; no expectation. + TestAndSetCondition condition = 5; + CreateIfMissing create_if_missing = 6; } message UpdateResponse { diff --git a/storage/src/vespa/storageapi/mbusprot/protocolserialization7.cpp b/storage/src/vespa/storageapi/mbusprot/protocolserialization7.cpp index 57047be6037..0f4a34cc775 100644 --- a/storage/src/vespa/storageapi/mbusprot/protocolserialization7.cpp +++ b/storage/src/vespa/storageapi/mbusprot/protocolserialization7.cpp @@ -465,6 +465,10 @@ void ProtocolSerialization7::onEncode(GBBuf& buf, const api::UpdateCommand& msg) if (msg.getCondition().isPresent()) { set_tas_condition(*req.mutable_condition(), msg.getCondition()); } + if (msg.has_cached_create_if_missing()) { + req.set_create_if_missing(msg.create_if_missing() ? protobuf::UpdateRequest_CreateIfMissing_TRUE + : protobuf::UpdateRequest_CreateIfMissing_FALSE); + } }); } @@ -482,6 +486,9 @@ api::StorageCommand::UP ProtocolSerialization7::onDecodeUpdateCommand(BBuf& buf) if (req.has_condition()) { cmd->setCondition(get_tas_condition(req.condition())); } + if (req.create_if_missing() != protobuf::UpdateRequest_CreateIfMissing_UNSPECIFIED) { + cmd->set_cached_create_if_missing(req.create_if_missing() == protobuf::UpdateRequest_CreateIfMissing_TRUE); + } return cmd; }); } diff --git a/storage/src/vespa/storageapi/message/persistence.cpp b/storage/src/vespa/storageapi/message/persistence.cpp index 4c24bb74faf..af054855bbe 100644 --- a/storage/src/vespa/storageapi/message/persistence.cpp +++ b/storage/src/vespa/storageapi/message/persistence.cpp @@ -105,13 +105,23 @@ UpdateCommand::UpdateCommand(const document::Bucket &bucket, const document::Doc : TestAndSetCommand(MessageType::UPDATE, bucket), _update(update), _timestamp(time), - _oldTimestamp(0) + _oldTimestamp(0), + _create_if_missing() { if ( ! _update) { throw vespalib::IllegalArgumentException("Cannot update a null update", VESPA_STRLOC); } } +bool +UpdateCommand::create_if_missing() const +{ + if (_create_if_missing.has_value()) { + return *_create_if_missing; + } + return _update->getCreateIfNonExistent(); +} + const document::DocumentType * UpdateCommand::getDocumentType() const { return &_update->getType(); diff --git a/storage/src/vespa/storageapi/message/persistence.h b/storage/src/vespa/storageapi/message/persistence.h index f44ab4e8280..0676e1d0f44 100644 --- a/storage/src/vespa/storageapi/message/persistence.h +++ b/storage/src/vespa/storageapi/message/persistence.h @@ -1,8 +1,6 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. /** - * @file persistence.h - * - * Persistence related commands, like put, get & remove + * Persistence related commands, like put, get & remove */ #pragma once @@ -10,6 +8,7 @@ #include <vespa/storageapi/defs.h> #include <vespa/document/base/documentid.h> #include <vespa/documentapi/messagebus/messages/testandsetcondition.h> +#include <optional> namespace document { class DocumentUpdate; @@ -117,20 +116,32 @@ class UpdateCommand : public TestAndSetCommand { std::shared_ptr<document::DocumentUpdate> _update; Timestamp _timestamp; Timestamp _oldTimestamp; + std::optional<bool> _create_if_missing; // caches the value held (possibly lazily deserialized) in _update public: UpdateCommand(const document::Bucket &bucket, const std::shared_ptr<document::DocumentUpdate>&, Timestamp); ~UpdateCommand() override; - void setTimestamp(Timestamp ts) { _timestamp = ts; } - void setOldTimestamp(Timestamp ts) { _oldTimestamp = ts; } + void setTimestamp(Timestamp ts) noexcept { _timestamp = ts; } + void setOldTimestamp(Timestamp ts) noexcept { _oldTimestamp = ts; } + + [[nodiscard]] bool has_cached_create_if_missing() const noexcept { + return _create_if_missing.has_value(); + } + // It is the caller's responsibility to ensure this value matches that of _update->getCreateIfNonExisting() + void set_cached_create_if_missing(bool create) noexcept { + _create_if_missing = create; + } const std::shared_ptr<document::DocumentUpdate>& getUpdate() const { return _update; } const document::DocumentId& getDocumentId() const override; Timestamp getTimestamp() const { return _timestamp; } Timestamp getOldTimestamp() const { return _oldTimestamp; } + // May throw iff has_cached_create_if_missing() == false, otherwise noexcept. + [[nodiscard]] bool create_if_missing() const; + const document::DocumentType * getDocumentType() const override; vespalib::string getSummary() const override; |