diff options
Diffstat (limited to 'storage')
7 files changed, 72 insertions, 29 deletions
diff --git a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp index 3c82931467e..a8771ddc28a 100644 --- a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp +++ b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp @@ -61,7 +61,8 @@ class TwoPhaseUpdateOperationTest : public CppUnit::TestFixture, CPPUNIT_TEST(testSafePathConditionMatchSendsPutsWithUpdatedDoc); CPPUNIT_TEST(testSafePathConditionParseFailureFailsWithIllegalParamsError); CPPUNIT_TEST(testSafePathConditonUnknownDocTypeFailsWithIllegalParamsError); - CPPUNIT_TEST(testSafePathConditionWithMissingDocFailsWithTasError); + CPPUNIT_TEST(safe_path_condition_with_missing_doc_and_no_auto_create_fails_with_tas_error); + CPPUNIT_TEST(safe_path_condition_with_missing_doc_and_auto_create_sends_puts); CPPUNIT_TEST(testFastPathCloseEdgeSendsCorrectReply); CPPUNIT_TEST(testSafePathCloseEdgeSendsCorrectReply); CPPUNIT_TEST_SUITE_END(); @@ -97,7 +98,8 @@ protected: void testSafePathConditionMatchSendsPutsWithUpdatedDoc(); void testSafePathConditionParseFailureFailsWithIllegalParamsError(); void testSafePathConditonUnknownDocTypeFailsWithIllegalParamsError(); - void testSafePathConditionWithMissingDocFailsWithTasError(); + void safe_path_condition_with_missing_doc_and_no_auto_create_fails_with_tas_error(); + void safe_path_condition_with_missing_doc_and_auto_create_sends_puts(); void testFastPathCloseEdgeSendsCorrectReply(); void testSafePathCloseEdgeSendsCorrectReply(); @@ -1096,7 +1098,7 @@ TwoPhaseUpdateOperationTest::testSafePathConditonUnknownDocTypeFailsWithIllegalP } void -TwoPhaseUpdateOperationTest::testSafePathConditionWithMissingDocFailsWithTasError() +TwoPhaseUpdateOperationTest::safe_path_condition_with_missing_doc_and_no_auto_create_fails_with_tas_error() { setupDistributor(2, 2, "storage:2 distributor:1"); std::shared_ptr<TwoPhaseUpdateOperation> cb( @@ -1118,6 +1120,22 @@ TwoPhaseUpdateOperationTest::testSafePathConditionWithMissingDocFailsWithTasErro } void +TwoPhaseUpdateOperationTest::safe_path_condition_with_missing_doc_and_auto_create_sends_puts() +{ + setupDistributor(2, 2, "storage:2 distributor:1"); + std::shared_ptr<TwoPhaseUpdateOperation> cb( + sendUpdate("0=1/2/3,1=2/3/4", UpdateOptions() + .condition("testdoctype1.headerval==120") + .createIfNonExistent(true))); + + MessageSenderStub sender; + cb->start(sender, framework::MilliSecTime(0)); + replyToGet(*cb, sender, 0, 100, false); + replyToGet(*cb, sender, 1, 110, false); + CPPUNIT_ASSERT_EQUAL("Put => 1,Put => 0"s, sender.getCommands(true, false, 2)); +} + +void TwoPhaseUpdateOperationTest::assertAbortedUpdateReplyWithContextPresent( const MessageSenderStub& closeSender) const { @@ -1177,9 +1195,6 @@ TwoPhaseUpdateOperationTest::testSafePathCloseEdgeSendsCorrectReply() // document IDs without explicit doctypes will _not_ be auto-failed on the // distributor. -// XXX shouldn't be necessary to have any special handling of create-if... and -// test-and-set right? They appear fully mutually exclusive. - // XXX: test case where update reply has been sent but callback still // has pending messages (e.g. n-of-m case). diff --git a/storage/src/tests/persistence/testandsettest.cpp b/storage/src/tests/persistence/testandsettest.cpp index 686e10ba5ef..197aa95fc22 100644 --- a/storage/src/tests/persistence/testandsettest.cpp +++ b/storage/src/tests/persistence/testandsettest.cpp @@ -66,8 +66,10 @@ public: void conditional_remove_executed_on_condition_match(); void conditional_update_not_executed_on_condition_mismatch(); void conditional_update_executed_on_condition_match(); + void conditional_update_not_executed_when_no_document_and_no_auto_create(); + void conditional_update_executed_when_no_document_but_auto_create_is_enabled(); void invalid_document_selection_should_fail(); - void non_existing_document_should_fail(); + void conditional_put_to_non_existing_document_should_fail(); void document_with_no_type_should_fail(); CPPUNIT_TEST_SUITE(TestAndSetTest); @@ -77,16 +79,17 @@ public: CPPUNIT_TEST(conditional_remove_executed_on_condition_match); CPPUNIT_TEST(conditional_update_not_executed_on_condition_mismatch); CPPUNIT_TEST(conditional_update_executed_on_condition_match); + CPPUNIT_TEST(conditional_update_not_executed_when_no_document_and_no_auto_create); + CPPUNIT_TEST(conditional_update_executed_when_no_document_but_auto_create_is_enabled); CPPUNIT_TEST(invalid_document_selection_should_fail); - CPPUNIT_TEST(non_existing_document_should_fail); + CPPUNIT_TEST(conditional_put_to_non_existing_document_should_fail); CPPUNIT_TEST(document_with_no_type_should_fail); CPPUNIT_TEST_SUITE_END(); protected: std::unique_ptr<api::UpdateCommand> conditional_update_test( - bool matchingHeader, - api::Timestamp timestampOne, - api::Timestamp timestampTwo); + bool createIfMissing, + api::Timestamp updateTimestamp); document::Document::SP createTestDocument(); document::Document::SP retrieveTestDocument(); @@ -183,18 +186,16 @@ void TestAndSetTest::conditional_remove_executed_on_condition_match() } std::unique_ptr<api::UpdateCommand> TestAndSetTest::conditional_update_test( - bool matchingHeader, - api::Timestamp timestampOne, - api::Timestamp timestampTwo) + bool createIfMissing, + api::Timestamp updateTimestamp) { - putTestDocument(matchingHeader, timestampOne); - auto docUpdate = std::make_shared<document::DocumentUpdate>(_env->_testDocMan.getTypeRepo(), testDoc->getType(), testDocId); auto fieldUpdate = document::FieldUpdate(testDoc->getField("content")); fieldUpdate.addUpdate(document::AssignValueUpdate(NEW_CONTENT)); docUpdate->addUpdate(fieldUpdate); + docUpdate->setCreateIfNonExistent(createIfMissing); - auto updateUp = std::make_unique<api::UpdateCommand>(makeDocumentBucket(BUCKET_ID), docUpdate, timestampTwo); + auto updateUp = std::make_unique<api::UpdateCommand>(makeDocumentBucket(BUCKET_ID), docUpdate, updateTimestamp); setTestCondition(*updateUp); return updateUp; } @@ -203,11 +204,12 @@ void TestAndSetTest::conditional_update_not_executed_on_condition_mismatch() { api::Timestamp timestampOne = 0; api::Timestamp timestampTwo = 1; - auto updateUp = conditional_update_test(false, timestampOne, timestampTwo); + putTestDocument(false, timestampOne); + auto updateUp = conditional_update_test(false, timestampTwo); CPPUNIT_ASSERT(thread->handleUpdate(*updateUp)->getResult() == api::ReturnCode::Result::TEST_AND_SET_CONDITION_FAILED); CPPUNIT_ASSERT_EQUAL(expectedDocEntryString(timestampOne, testDocId), - dumpBucket(BUCKET_ID)); + dumpBucket(BUCKET_ID)); assertTestDocumentFoundAndMatchesContent(OLD_CONTENT); } @@ -216,7 +218,8 @@ void TestAndSetTest::conditional_update_executed_on_condition_match() { api::Timestamp timestampOne = 0; api::Timestamp timestampTwo = 1; - auto updateUp = conditional_update_test(true, timestampOne, timestampTwo); + putTestDocument(true, timestampOne); + auto updateUp = conditional_update_test(false, timestampTwo); CPPUNIT_ASSERT(thread->handleUpdate(*updateUp)->getResult() == api::ReturnCode::Result::OK); CPPUNIT_ASSERT_EQUAL(expectedDocEntryString(timestampOne, testDocId) + @@ -226,6 +229,23 @@ void TestAndSetTest::conditional_update_executed_on_condition_match() assertTestDocumentFoundAndMatchesContent(NEW_CONTENT); } +void TestAndSetTest::conditional_update_not_executed_when_no_document_and_no_auto_create() { + api::Timestamp updateTimestamp = 200; + auto updateUp = conditional_update_test(false, updateTimestamp); + + CPPUNIT_ASSERT(thread->handleUpdate(*updateUp)->getResult() == api::ReturnCode::Result::TEST_AND_SET_CONDITION_FAILED); + CPPUNIT_ASSERT_EQUAL(""s, dumpBucket(BUCKET_ID)); +} + +void TestAndSetTest::conditional_update_executed_when_no_document_but_auto_create_is_enabled() { + api::Timestamp updateTimestamp = 200; + auto updateUp = conditional_update_test(true, updateTimestamp); + + CPPUNIT_ASSERT(thread->handleUpdate(*updateUp)->getResult() == api::ReturnCode::Result::OK); + CPPUNIT_ASSERT_EQUAL(expectedDocEntryString(updateTimestamp, testDocId), dumpBucket(BUCKET_ID)); + assertTestDocumentFoundAndMatchesContent(NEW_CONTENT); +} + void TestAndSetTest::invalid_document_selection_should_fail() { // Conditionally replace nonexisting document @@ -238,7 +258,7 @@ void TestAndSetTest::invalid_document_selection_should_fail() CPPUNIT_ASSERT_EQUAL(""s, dumpBucket(BUCKET_ID)); } -void TestAndSetTest::non_existing_document_should_fail() +void TestAndSetTest::conditional_put_to_non_existing_document_should_fail() { // Conditionally replace nonexisting document // Fail since no document exists to match with test and set diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp index c652f787b2e..b7ebafc114c 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp @@ -378,7 +378,7 @@ TwoPhaseUpdateOperation::handleSafePathReceivedGet(DistributorMessageSender& sen } docToUpdate = reply.getDocument(); setUpdatedForTimestamp(receivedTimestamp); - } else if (hasTasCondition()) { + } else if (hasTasCondition() && !shouldCreateIfNonExistent()) { replyWithTasFailure(sender, "Document did not exist"); return; } else if (shouldCreateIfNonExistent()) { diff --git a/storage/src/vespa/storage/persistence/persistencethread.cpp b/storage/src/vespa/storage/persistence/persistencethread.cpp index 1b221ea0b7c..55f7c076244 100644 --- a/storage/src/vespa/storage/persistence/persistencethread.cpp +++ b/storage/src/vespa/storage/persistence/persistencethread.cpp @@ -85,9 +85,10 @@ bool PersistenceThread::tasConditionExists(const api::TestAndSetCommand & cmd) { return cmd.getCondition().isPresent(); } -bool PersistenceThread::tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker) { +bool PersistenceThread::tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker, + bool missingDocumentImpliesMatch) { try { - TestAndSetHelper helper(*this, cmd); + TestAndSetHelper helper(*this, cmd, missingDocumentImpliesMatch); auto code = helper.retrieveAndMatch(); if (code.failed()) { @@ -149,7 +150,7 @@ PersistenceThread::handleUpdate(api::UpdateCommand& cmd) auto tracker = std::make_unique<MessageTracker>(metrics, _env._component.getClock()); metrics.request_size.addValue(cmd.getApproxByteSize()); - if (tasConditionExists(cmd) && !tasConditionMatches(cmd, *tracker)) { + if (tasConditionExists(cmd) && !tasConditionMatches(cmd, *tracker, cmd.getUpdate()->getCreateIfNonExistent())) { return tracker; } diff --git a/storage/src/vespa/storage/persistence/persistencethread.h b/storage/src/vespa/storage/persistence/persistencethread.h index 05ca34f623d..ed27a759e8b 100644 --- a/storage/src/vespa/storage/persistence/persistencethread.h +++ b/storage/src/vespa/storage/persistence/persistencethread.h @@ -87,7 +87,8 @@ private: friend class TestAndSetHelper; bool tasConditionExists(const api::TestAndSetCommand & cmd); - bool tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker); + bool tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker, + bool missingDocumentImpliesMatch = false); }; } // storage diff --git a/storage/src/vespa/storage/persistence/testandsethelper.cpp b/storage/src/vespa/storage/persistence/testandsethelper.cpp index 70a6bdf21db..511d44ad331 100644 --- a/storage/src/vespa/storage/persistence/testandsethelper.cpp +++ b/storage/src/vespa/storage/persistence/testandsethelper.cpp @@ -39,11 +39,13 @@ spi::GetResult TestAndSetHelper::retrieveDocument(const document::FieldSet & fie _thread._context); } -TestAndSetHelper::TestAndSetHelper(PersistenceThread & thread, const api::TestAndSetCommand & cmd) +TestAndSetHelper::TestAndSetHelper(PersistenceThread & thread, const api::TestAndSetCommand & cmd, + bool missingDocumentImpliesMatch) : _thread(thread), _component(thread._env._component), _cmd(cmd), - _docId(cmd.getDocumentId()) + _docId(cmd.getDocumentId()), + _missingDocumentImpliesMatch(missingDocumentImpliesMatch) { getDocumentType(); parseDocumentSelection(); @@ -69,6 +71,8 @@ api::ReturnCode TestAndSetHelper::retrieveAndMatch() { // Document matches return api::ReturnCode(); + } else if (_missingDocumentImpliesMatch) { + return api::ReturnCode(); } return api::ReturnCode(api::ReturnCode::TEST_AND_SET_CONDITION_FAILED, "Document does not exist"); diff --git a/storage/src/vespa/storage/persistence/testandsethelper.h b/storage/src/vespa/storage/persistence/testandsethelper.h index 4e48259bc79..21c111c712f 100644 --- a/storage/src/vespa/storage/persistence/testandsethelper.h +++ b/storage/src/vespa/storage/persistence/testandsethelper.h @@ -26,13 +26,15 @@ class TestAndSetHelper { const document::DocumentId _docId; const document::DocumentType * _docTypePtr; std::unique_ptr<document::select::Node> _docSelectionUp; + bool _missingDocumentImpliesMatch; void getDocumentType(); void parseDocumentSelection(); spi::GetResult retrieveDocument(const document::FieldSet & fieldSet); public: - TestAndSetHelper(PersistenceThread & thread, const api::TestAndSetCommand & cmd); + TestAndSetHelper(PersistenceThread & thread, const api::TestAndSetCommand & cmd, + bool missingDocumentImpliesMatch = false); ~TestAndSetHelper(); api::ReturnCode retrieveAndMatch(); }; |