diff options
Diffstat (limited to 'storage/src/vespa/storage')
6 files changed, 115 insertions, 67 deletions
diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp index 515b72520ec..302845b2b8d 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp @@ -568,12 +568,8 @@ TwoPhaseUpdateOperation::handleSafePathReceivedGet(DistributorStripeMessageSende } docToUpdate = reply.getDocument(); setUpdatedForTimestamp(receivedTimestamp); - } else if (hasTasCondition() && !shouldCreateIfNonExistent()) { - replyWithTasFailure(sender, "Document did not exist"); - return; } else if (shouldCreateIfNonExistent()) { - LOG(debug, "No existing documents found for %s, creating blank document to update", - update_doc_id().c_str()); + LOG(debug, "No existing documents found for %s, creating blank document to update", update_doc_id().c_str()); docToUpdate = createBlankDocument(); setUpdatedForTimestamp(putTimestamp); } else { diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp index e20c0475556..85a13f5f0db 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.cpp +++ b/storage/src/vespa/storage/persistence/asynchandler.cpp @@ -115,7 +115,7 @@ bucketStatesAreSemanticallyEqual(const api::BucketInfo& a, const api::BucketInfo class UnrevertableRemoveEntryProcessor : public BucketProcessor::EntryProcessor { public: using DocumentIdsAndTimeStamps = std::vector<spi::IdAndTimestamp>; - UnrevertableRemoveEntryProcessor(DocumentIdsAndTimeStamps & to_remove) + explicit UnrevertableRemoveEntryProcessor(DocumentIdsAndTimeStamps& to_remove) noexcept : _to_remove(to_remove) {} @@ -159,11 +159,15 @@ AsyncHandler::handlePut(api::PutCommand& cmd, MessageTracker::UP trackerUP) cons tracker.setMetric(metrics); metrics.request_size.addValue(cmd.getApproxByteSize()); - if (tasConditionExists(cmd) && !tasConditionMatches(cmd, tracker, tracker.context())) { - // Will also count condition parse failures etc as TaS failures, but - // those results _will_ increase the error metrics as well. - metrics.test_and_set_failed.inc(); - return trackerUP; + if (tasConditionExists(cmd)) { + auto tas_res = tasConditionMatches(cmd, tracker, tracker.context(), DocNotFoundPolicy::ReturnTaSError); + if (tas_res == TasResult::FailedAndSetInTracker) { + // Will also count condition parse failures etc. as TaS failures, but + // those results _will_ increase the error metrics as well. + metrics.test_and_set_failed.inc(); + return trackerUP; + } + assert(tas_res == TasResult::Matched); // NotFound treated as failure for Puts } spi::Bucket bucket = _env.getBucket(cmd.getDocumentId(), cmd.getBucket()); @@ -283,9 +287,19 @@ AsyncHandler::handleUpdate(api::UpdateCommand& cmd, MessageTracker::UP trackerUP tracker.setMetric(metrics); metrics.request_size.addValue(cmd.getApproxByteSize()); - if (tasConditionExists(cmd) && !tasConditionMatches(cmd, tracker, tracker.context(), cmd.getUpdate()->getCreateIfNonExistent())) { - metrics.test_and_set_failed.inc(); - return trackerUP; + if (tasConditionExists(cmd)) { + const bool create_missing = cmd.getUpdate()->getCreateIfNonExistent(); + const auto not_found_policy = (create_missing ? DocNotFoundPolicy::TreatAsMatch : DocNotFoundPolicy::ReturnNotFound); + auto tas_res = tasConditionMatches(cmd, tracker, tracker.context(), not_found_policy); + if (tas_res == TasResult::FailedAndSetInTracker) { + metrics.test_and_set_failed.inc(); + return trackerUP; + } else if (tas_res == TasResult::NotFound) { + metrics.notFound.inc(); + tracker.setReply(std::make_shared<api::UpdateReply>(cmd, 0)); // Zero-timestamp == not found + return trackerUP; + } + assert(tas_res == TasResult::Matched); } spi::Bucket bucket = _env.getBucket(cmd.getDocumentId(), cmd.getBucket()); @@ -313,9 +327,13 @@ AsyncHandler::handleRemove(api::RemoveCommand& cmd, MessageTracker::UP trackerUP tracker.setMetric(metrics); metrics.request_size.addValue(cmd.getApproxByteSize()); - if (tasConditionExists(cmd) && !tasConditionMatches(cmd, tracker, tracker.context())) { - metrics.test_and_set_failed.inc(); - return trackerUP; + if (tasConditionExists(cmd)) { + auto tas_res = tasConditionMatches(cmd, tracker, tracker.context(), DocNotFoundPolicy::ReturnTaSError); + if (tas_res == TasResult::FailedAndSetInTracker) { + metrics.test_and_set_failed.inc(); + return trackerUP; + } + assert(tas_res == TasResult::Matched); // NotFound treated as failure for Removes } spi::Bucket bucket = _env.getBucket(cmd.getDocumentId(), cmd.getBucket()); @@ -354,24 +372,26 @@ AsyncHandler::tasConditionExists(const api::TestAndSetCommand & cmd) { return cmd.getCondition().isPresent(); } -bool -AsyncHandler::tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker, - spi::Context & context, bool missingDocumentImpliesMatch) const { +AsyncHandler::TasResult +AsyncHandler::tasConditionMatches(const api::TestAndSetCommand& cmd, MessageTracker& tracker, + spi::Context& context, DocNotFoundPolicy doc_not_found_policy) const { try { - TestAndSetHelper helper(_env, _spi, _bucketIdFactory, cmd, missingDocumentImpliesMatch); - - auto code = helper.retrieveAndMatch(context); - if (code.failed()) { - tracker.fail(code.getResult(), code.getMessage()); - return false; + TestAndSetHelper helper(_env, _spi, _bucketIdFactory, cmd, doc_not_found_policy); + + auto maybe_code = helper.retrieveAndMatch(context); + if (maybe_code && maybe_code->failed()) { + tracker.fail(maybe_code->getResult(), maybe_code->getMessage()); + return TasResult::FailedAndSetInTracker; + } else if (!maybe_code) { + return TasResult::NotFound; } } catch (const TestAndSetException & e) { auto code = e.getCode(); tracker.fail(code.getResult(), code.getMessage()); - return false; + return TasResult::FailedAndSetInTracker; } - return true; + return TasResult::Matched; } bool diff --git a/storage/src/vespa/storage/persistence/asynchandler.h b/storage/src/vespa/storage/persistence/asynchandler.h index c5122647caa..c68fa962361 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.h +++ b/storage/src/vespa/storage/persistence/asynchandler.h @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once +#include "doc_not_found_policy.h" #include "messages.h" #include <vespa/storageapi/message/persistence.h> #include <vespa/storageapi/message/removelocation.h> @@ -38,8 +39,15 @@ public: private: bool checkProviderBucketInfoMatches(const spi::Bucket&, const api::BucketInfo&) const; static bool tasConditionExists(const api::TestAndSetCommand & cmd); - bool tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker & tracker, - spi::Context & context, bool missingDocumentImpliesMatch = false) const; + + enum class TasResult { + Matched, + FailedAndSetInTracker, + NotFound + }; + + TasResult tasConditionMatches(const api::TestAndSetCommand& cmd, MessageTracker& tracker, + spi::Context& context, DocNotFoundPolicy doc_not_found_policy) const; const PersistenceUtil & _env; spi::PersistenceProvider & _spi; BucketOwnershipNotifier & _bucketOwnershipNotifier; diff --git a/storage/src/vespa/storage/persistence/doc_not_found_policy.h b/storage/src/vespa/storage/persistence/doc_not_found_policy.h new file mode 100644 index 00000000000..27a96ecc179 --- /dev/null +++ b/storage/src/vespa/storage/persistence/doc_not_found_policy.h @@ -0,0 +1,14 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#pragma once + +namespace storage { + +// Specifies the semantics of a test-and-set comparison where the underlying +// document does not exist in the backing store. +enum class DocNotFoundPolicy { + ReturnTaSError, // Return an explicit test-and-set failure return code (_not_ a not-found error) + TreatAsMatch, // Act as if the document _did_ exist and the condition matched it + ReturnNotFound // Explicitly propagate up the fact that the document did not exist +}; + +} diff --git a/storage/src/vespa/storage/persistence/testandsethelper.cpp b/storage/src/vespa/storage/persistence/testandsethelper.cpp index 393dac09f72..1f5661a7813 100644 --- a/storage/src/vespa/storage/persistence/testandsethelper.cpp +++ b/storage/src/vespa/storage/persistence/testandsethelper.cpp @@ -15,13 +15,15 @@ using namespace std::string_literals; namespace storage { void TestAndSetHelper::resolveDocumentType(const document::DocumentTypeRepo & documentTypeRepo) { - if (_docTypePtr != nullptr) return; - if (!_docId.hasDocType()) { + if (_doc_type_ptr != nullptr) { + return; + } + if (!_doc_id.hasDocType()) { throw TestAndSetException(api::ReturnCode(api::ReturnCode::ILLEGAL_PARAMETERS, "Document id has no doctype")); } - _docTypePtr = documentTypeRepo.getDocumentType(_docId.getDocType()); - if (_docTypePtr == nullptr) { + _doc_type_ptr = documentTypeRepo.getDocumentType(_doc_id.getDocType()); + if (_doc_type_ptr == nullptr) { throw TestAndSetException(api::ReturnCode(api::ReturnCode::ILLEGAL_PARAMETERS, "Document type does not exist")); } } @@ -31,39 +33,41 @@ void TestAndSetHelper::parseDocumentSelection(const document::DocumentTypeRepo & document::select::Parser parser(documentTypeRepo, bucketIdFactory); try { - _docSelectionUp = parser.parse(_cmd.getCondition().getSelection()); + _doc_selection_up = parser.parse(_cmd.getCondition().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(_doc_id, _cmd.getBucket()), fieldSet, _cmd.getDocumentId(), 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 api::TestAndSetCommand& cmd, + DocNotFoundPolicy doc_not_found_policy) : _env(env), _spi(spi), _cmd(cmd), - _docId(cmd.getDocumentId()), - _docTypePtr(_cmd.getDocumentType()), - _missingDocumentImpliesMatch(missingDocumentImpliesMatch) + _doc_id(cmd.getDocumentId()), + _doc_type_ptr(_cmd.getDocumentType()), + _doc_not_found_policy(doc_not_found_policy) { - const auto & repo = _env.getDocumentTypeRepo(); + 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) { +std::optional<api::ReturnCode> +TestAndSetHelper::retrieveAndMatch(spi::Context& context) { // Walk document selection tree to build a minimal field set - FieldVisitor fieldVisitor(*_docTypePtr); + FieldVisitor fieldVisitor(*_doc_type_ptr); try { - _docSelectionUp->visit(fieldVisitor); + _doc_selection_up->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. " @@ -71,13 +75,12 @@ TestAndSetHelper::retrieveAndMatch(spi::Context & context) { 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) { + if (_doc_selection_up->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(), @@ -86,14 +89,15 @@ TestAndSetHelper::retrieveAndMatch(spi::Context & context) { // Document matches return api::ReturnCode(); - } else if (_missingDocumentImpliesMatch) { + } else if (_doc_not_found_policy == DocNotFoundPolicy::TreatAsMatch) { return api::ReturnCode(); + } else if (_doc_not_found_policy == DocNotFoundPolicy::ReturnTaSError) { + 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" : "")); } - - 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" : "")); + return std::nullopt; // Not found } } // storage diff --git a/storage/src/vespa/storage/persistence/testandsethelper.h b/storage/src/vespa/storage/persistence/testandsethelper.h index 82710e523c4..a4fcf9f9c5f 100644 --- a/storage/src/vespa/storage/persistence/testandsethelper.h +++ b/storage/src/vespa/storage/persistence/testandsethelper.h @@ -3,9 +3,11 @@ #pragma once +#include "doc_not_found_policy.h" #include <vespa/storageapi/message/persistence.h> #include <vespa/persistence/spi/result.h> #include <stdexcept> +#include <optional> namespace document::select { class Node; } namespace document { @@ -36,13 +38,13 @@ public: }; class TestAndSetHelper { - const PersistenceUtil &_env; - const spi::PersistenceProvider &_spi; - const api::TestAndSetCommand &_cmd; - const document::DocumentId _docId; - const document::DocumentType * _docTypePtr; - std::unique_ptr<document::select::Node> _docSelectionUp; - bool _missingDocumentImpliesMatch; + const PersistenceUtil& _env; + const spi::PersistenceProvider& _spi; + const api::TestAndSetCommand& _cmd; + const document::DocumentId _doc_id; + const document::DocumentType* _doc_type_ptr; + std::unique_ptr<document::select::Node> _doc_selection_up; + DocNotFoundPolicy _doc_not_found_policy; void resolveDocumentType(const document::DocumentTypeRepo & documentTypeRepo); void parseDocumentSelection(const document::DocumentTypeRepo & documentTypeRepo, @@ -50,11 +52,15 @@ 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); + TestAndSetHelper(const PersistenceUtil& env, const spi::PersistenceProvider& _spi, + const document::BucketIdFactory& bucket_id_factory, + const api::TestAndSetCommand& cmd, + DocNotFoundPolicy doc_not_found_policy = DocNotFoundPolicy::ReturnTaSError); ~TestAndSetHelper(); - api::ReturnCode retrieveAndMatch(spi::Context & context); + // If document was not found: + // iff doc_not_found_returns_tas_error == true, returns nullopt + // otherwise, returns a TaS error return code + std::optional<api::ReturnCode> retrieveAndMatch(spi::Context& context); }; } // storage |