diff options
14 files changed, 264 insertions, 282 deletions
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp index b8a390ed0ce..944217796ab 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp @@ -27,109 +27,108 @@ using document::FixedBucketSpaces; namespace storage::spi::dummy { -BucketContent::BucketContent() - : _entries(), - _gidMap(), - _info(), - _inUse(false), - _outdatedInfo(true), - _active(false) -{ } -BucketContent::~BucketContent() = default; - -uint32_t -BucketContent::computeEntryChecksum(const BucketEntry& e) const -{ - vespalib::crc_32_type checksummer; - - uint64_t ts(e.entry->getTimestamp()); - checksummer.process_bytes(&e.gid, sizeof(GlobalId)); - checksummer.process_bytes(&ts, sizeof(uint64_t)); - return checksummer.checksum(); -} - -BucketChecksum -BucketContent::updateRollingChecksum(uint32_t entryChecksum) -{ - uint32_t checksum = _info.getChecksum(); - checksum ^= entryChecksum; - if (checksum == 0) { - checksum = 1; + BucketContent::BucketContent() + : _entries(), + _gidMap(), + _info(), + _inUse(false), + _outdatedInfo(true), + _active(false) {} + + BucketContent::~BucketContent() = default; + + uint32_t + BucketContent::computeEntryChecksum(const BucketEntry &e) const { + vespalib::crc_32_type checksummer; + + uint64_t ts(e.entry->getTimestamp()); + checksummer.process_bytes(&e.gid, sizeof(GlobalId)); + checksummer.process_bytes(&ts, sizeof(uint64_t)); + return checksummer.checksum(); + } + + BucketChecksum + BucketContent::updateRollingChecksum(uint32_t entryChecksum) { + uint32_t checksum = _info.getChecksum(); + checksum ^= entryChecksum; + if (checksum == 0) { + checksum = 1; + } + return BucketChecksum(checksum); } - return BucketChecksum(checksum); -} -const BucketInfo& -BucketContent::getBucketInfo() const -{ - if (!_outdatedInfo) { - return _info; - } + const BucketInfo & + BucketContent::getBucketInfo() const { + if (!_outdatedInfo) { + return _info; + } - // Checksum should only depend on the newest entry for each document that - // has not been removed. - uint32_t unique = 0; - uint32_t uniqueSize = 0; - uint32_t totalSize = 0; - uint32_t checksum = 0; + // Checksum should only depend on the newest entry for each document that + // has not been removed. + uint32_t unique = 0; + uint32_t uniqueSize = 0; + uint32_t totalSize = 0; + uint32_t checksum = 0; - for (const BucketEntry & bucketEntry : _entries) { - const DocEntry& entry(*bucketEntry.entry); - const GlobalId& gid(bucketEntry.gid); + for (const BucketEntry &bucketEntry: _entries) { + const DocEntry &entry(*bucketEntry.entry); + const GlobalId &gid(bucketEntry.gid); - GidMapType::const_iterator gidIt(_gidMap.find(gid)); - assert(gidIt != _gidMap.end()); + GidMapType::const_iterator gidIt(_gidMap.find(gid)); + assert(gidIt != _gidMap.end()); - totalSize += entry.getSize(); - if (entry.isRemove()) { - continue; + totalSize += entry.getSize(); + if (entry.isRemove()) { + continue; + } + // Only include if we're newest entry for the particular GID + if (gidIt->second.get() != &entry) { + continue; + } + ++unique; + uniqueSize += entry.getSize(); + + checksum ^= computeEntryChecksum(bucketEntry); } - // Only include if we're newest entry for the particular GID - if (gidIt->second.get() != &entry) { - continue; + if (!unique) { + checksum = 0; + } else if (checksum == 0) { + checksum = 1; } - ++unique; - uniqueSize += entry.getSize(); - checksum ^= computeEntryChecksum(bucketEntry); - } - if (!unique) { - checksum = 0; - } else if (checksum == 0) { - checksum = 1; - } + _info = BucketInfo(BucketChecksum(checksum), + unique, + uniqueSize, + _entries.size(), + totalSize, + BucketInfo::READY, + _active ? BucketInfo::ACTIVE : BucketInfo::NOT_ACTIVE); - _info = BucketInfo(BucketChecksum(checksum), - unique, - uniqueSize, - _entries.size(), - totalSize, - BucketInfo::READY, - _active ? BucketInfo::ACTIVE : BucketInfo::NOT_ACTIVE); + _outdatedInfo = false; + return _info; + } - _outdatedInfo = false; - return _info; -} + namespace { -namespace { + struct TimestampLess { + bool operator()(const BucketEntry &bucketEntry, Timestamp t) { + return bucketEntry.entry->getTimestamp() < t; + } -struct TimestampLess { - bool operator()(const BucketEntry &bucketEntry, Timestamp t) - { return bucketEntry.entry->getTimestamp() < t; } - bool operator()(Timestamp t, const BucketEntry &bucketEntry) - { return t < bucketEntry.entry->getTimestamp(); } -}; + bool operator()(Timestamp t, const BucketEntry &bucketEntry) { + return t < bucketEntry.entry->getTimestamp(); + } + }; -} // namespace + } // namespace -bool -BucketContent::hasTimestamp(Timestamp t) const -{ - if (!_entries.empty() && _entries.back().entry->getTimestamp() < t) { - return false; + bool + BucketContent::hasTimestamp(Timestamp t) const { + if (!_entries.empty() && _entries.back().entry->getTimestamp() < t) { + return false; + } + return binary_search(_entries.begin(), _entries.end(), t, TimestampLess()); } - return binary_search(_entries.begin(), _entries.end(), t, TimestampLess()); -} /** * GID map semantics: @@ -147,181 +146,174 @@ BucketContent::hasTimestamp(Timestamp t) const * document), we can remove the mapping entirely. */ -void -BucketContent::insert(DocEntry::SP e) -{ - LOG(spam, "insert(%s)", e->toString().c_str()); - const DocumentId* docId(e->getDocumentId()); - assert(docId != 0); - GlobalId gid(docId->getGlobalId()); - GidMapType::iterator gidIt(_gidMap.find(gid)); - - if (!_entries.empty() && - _entries.back().entry->getTimestamp() < e->getTimestamp()) { - _entries.push_back(BucketEntry(e, gid)); - } else { - std::vector<BucketEntry>::iterator it = - lower_bound(_entries.begin(), - _entries.end(), - e->getTimestamp(), - TimestampLess()); - if (it != _entries.end()) { - if (it->entry->getTimestamp() == e->getTimestamp()) { - if (*it->entry.get() == *e) { - LOG(debug, "Ignoring duplicate put entry %s", - e->toString().c_str()); - return; - } else { - LOG(error, "Entry %s was already present." - "Was trying to insert %s.", - it->entry->toString().c_str(), - e->toString().c_str()); - LOG_ABORT("should not reach here"); + void + BucketContent::insert(DocEntry::SP e) { + LOG(spam, "insert(%s)", e->toString().c_str()); + const DocumentId *docId(e->getDocumentId()); + assert(docId != 0); + GlobalId gid(docId->getGlobalId()); + GidMapType::iterator gidIt(_gidMap.find(gid)); + + if (!_entries.empty() && + _entries.back().entry->getTimestamp() < e->getTimestamp()) { + _entries.push_back(BucketEntry(e, gid)); + } else { + std::vector<BucketEntry>::iterator it = + lower_bound(_entries.begin(), + _entries.end(), + e->getTimestamp(), + TimestampLess()); + if (it != _entries.end()) { + if (it->entry->getTimestamp() == e->getTimestamp()) { + if (*it->entry.get() == *e) { + LOG(debug, "Ignoring duplicate put entry %s", + e->toString().c_str()); + return; + } else { + LOG(error, "Entry %s was already present." + "Was trying to insert %s.", + it->entry->toString().c_str(), + e->toString().c_str()); + LOG_ABORT("should not reach here"); + } } } + _entries.insert(it, BucketEntry(e, gid)); } - _entries.insert(it, BucketEntry(e, gid)); - } - // GID map points to newest entry for that particular GID - if (gidIt != _gidMap.end()) { - if (gidIt->second->getTimestamp() < e->getTimestamp()) { - // TODO(vekterli): add support for cheap info updates for putting - // newer versions of a document etc. by XORing away old checksum. - gidIt->second = e; - } else { - LOG(spam, - "Newly inserted entry %s was older than existing entry %s; " - "not updating GID mapping", - e->toString().c_str(), - gidIt->second->toString().c_str()); - } - _outdatedInfo = true; - } else { - _gidMap.insert(GidMapType::value_type(gid, e)); - // Since GID didn't exist before, it means we can do a running - // update of the bucket info. Bucket checksum is XOR of all entry - // checksums, which is commutative. - // Only bother to update if we don't have to re-do it all afterwards - // anyway. - // Updating bucketinfo before we update entries since we assume rest - // of function is nothrow. - if (!_outdatedInfo) { - if (!e->isRemove()) { - _info = BucketInfo(updateRollingChecksum( - computeEntryChecksum(BucketEntry(e, gid))), - _info.getDocumentCount() + 1, - _info.getDocumentSize() + e->getSize(), - _info.getEntryCount() + 1, - _info.getUsedSize() + e->getSize(), - _info.getReady(), - _info.getActive()); + // GID map points to newest entry for that particular GID + if (gidIt != _gidMap.end()) { + if (gidIt->second->getTimestamp() < e->getTimestamp()) { + // TODO(vekterli): add support for cheap info updates for putting + // newer versions of a document etc. by XORing away old checksum. + gidIt->second = e; } else { - _info = BucketInfo(_info.getChecksum(), - _info.getDocumentCount(), - _info.getDocumentSize(), - _info.getEntryCount() + 1, - _info.getUsedSize() + e->getSize(), - _info.getReady(), - _info.getActive()); + LOG(spam, + "Newly inserted entry %s was older than existing entry %s; " + "not updating GID mapping", + e->toString().c_str(), + gidIt->second->toString().c_str()); } + _outdatedInfo = true; + } else { + _gidMap.insert(GidMapType::value_type(gid, e)); + // Since GID didn't exist before, it means we can do a running + // update of the bucket info. Bucket checksum is XOR of all entry + // checksums, which is commutative. + // Only bother to update if we don't have to re-do it all afterwards + // anyway. + // Updating bucketinfo before we update entries since we assume rest + // of function is nothrow. + if (!_outdatedInfo) { + if (!e->isRemove()) { + _info = BucketInfo(updateRollingChecksum( + computeEntryChecksum(BucketEntry(e, gid))), + _info.getDocumentCount() + 1, + _info.getDocumentSize() + e->getSize(), + _info.getEntryCount() + 1, + _info.getUsedSize() + e->getSize(), + _info.getReady(), + _info.getActive()); + } else { + _info = BucketInfo(_info.getChecksum(), + _info.getDocumentCount(), + _info.getDocumentSize(), + _info.getEntryCount() + 1, + _info.getUsedSize() + e->getSize(), + _info.getReady(), + _info.getActive()); + } - LOG(spam, - "After cheap bucketinfo update, state is %s (inserted %s)", - _info.toString().c_str(), - e->toString().c_str()); + LOG(spam, + "After cheap bucketinfo update, state is %s (inserted %s)", + _info.toString().c_str(), + e->toString().c_str()); + } } - } - - assert(_outdatedInfo || _info.getEntryCount() == _entries.size()); -} -DocEntry::SP -BucketContent::getEntry(const DocumentId& did) const -{ - GidMapType::const_iterator it(_gidMap.find(did.getGlobalId())); - if (it != _gidMap.end()) { - return it->second; + assert(_outdatedInfo || _info.getEntryCount() == _entries.size()); } - return DocEntry::SP(); -} - -DocEntry::SP -BucketContent::getEntry(Timestamp t) const -{ - std::vector<BucketEntry>::const_iterator iter = - lower_bound(_entries.begin(), _entries.end(), t, TimestampLess()); - if (iter == _entries.end() || iter->entry->getTimestamp() != t) { + DocEntry::SP + BucketContent::getEntry(const DocumentId &did) const { + GidMapType::const_iterator it(_gidMap.find(did.getGlobalId())); + if (it != _gidMap.end()) { + return it->second; + } return DocEntry::SP(); - } else { - return iter->entry; } -} -void -BucketContent::eraseEntry(Timestamp t) -{ - std::vector<BucketEntry>::iterator iter = - lower_bound(_entries.begin(), _entries.end(), t, TimestampLess()); - - if (iter != _entries.end() && iter->entry->getTimestamp() == t) { - assert(iter->entry->getDocumentId() != 0); - GidMapType::iterator gidIt( - _gidMap.find(iter->entry->getDocumentId()->getGlobalId())); - assert(gidIt != _gidMap.end()); - _entries.erase(iter); - if (gidIt->second->getTimestamp() == t) { - LOG(debug, "erasing timestamp %" PRIu64 " from GID map", t.getValue()); - // TODO(vekterli): O(1) bucket info update for this case - // FIXME: is this correct? seems like it could cause wrong behavior! - _gidMap.erase(gidIt); - } // else: not erasing newest entry, cannot erase from GID map - _outdatedInfo = true; - } -} - -DummyPersistence::DummyPersistence(const std::shared_ptr<const document::DocumentTypeRepo>& repo) - : _initialized(false), - _repo(repo), - _content(), - _nextIterator(1), - _iterators(), - _monitor(), - _clusterState() -{} + DocEntry::SP + BucketContent::getEntry(Timestamp t) const { + std::vector<BucketEntry>::const_iterator iter = + lower_bound(_entries.begin(), _entries.end(), t, TimestampLess()); -DummyPersistence::~DummyPersistence() = default; + if (iter == _entries.end() || iter->entry->getTimestamp() != t) { + return DocEntry::SP(); + } else { + return iter->entry; + } + } -document::select::Node::UP -DummyPersistence::parseDocumentSelection(const string& documentSelection, bool allowLeaf) -{ - document::select::Node::UP ret; - try { - document::select::Parser parser(*_repo, document::BucketIdFactory()); - ret = parser.parse(documentSelection); - } catch (document::select::ParsingFailedException& e) { - return document::select::Node::UP(); + void + BucketContent::eraseEntry(Timestamp t) { + std::vector<BucketEntry>::iterator iter = + lower_bound(_entries.begin(), _entries.end(), t, TimestampLess()); + + if (iter != _entries.end() && iter->entry->getTimestamp() == t) { + assert(iter->entry->getDocumentId() != 0); + GidMapType::iterator gidIt( + _gidMap.find(iter->entry->getDocumentId()->getGlobalId())); + assert(gidIt != _gidMap.end()); + _entries.erase(iter); + if (gidIt->second->getTimestamp() == t) { + LOG(debug, "erasing timestamp %" PRIu64 " from GID map", t.getValue()); + // TODO(vekterli): O(1) bucket info update for this case + // FIXME: is this correct? seems like it could cause wrong behavior! + _gidMap.erase(gidIt); + } // else: not erasing newest entry, cannot erase from GID map + _outdatedInfo = true; + } } - if (ret->isLeafNode() && !allowLeaf) { - return document::select::Node::UP(); + + DummyPersistence::DummyPersistence(const std::shared_ptr<const document::DocumentTypeRepo> &repo) + : _initialized(false), + _repo(repo), + _content(), + _nextIterator(1), + _iterators(), + _monitor(), + _clusterState() {} + + DummyPersistence::~DummyPersistence() = default; + + document::select::Node::UP + DummyPersistence::parseDocumentSelection(const string &documentSelection, bool allowLeaf) { + document::select::Node::UP ret; + try { + document::select::Parser parser(*_repo, document::BucketIdFactory()); + ret = parser.parse(documentSelection); + } catch (document::select::ParsingFailedException &e) { + return document::select::Node::UP(); + } + if (ret->isLeafNode() && !allowLeaf) { + return document::select::Node::UP(); + } + return ret; } - return ret; -} -Result -DummyPersistence::initialize() -{ - assert(!_initialized); - _initialized = true; - return Result(); -} + Result + DummyPersistence::initialize() { + assert(!_initialized); + _initialized = true; + return Result(); + } #define DUMMYPERSISTENCE_VERIFY_INITIALIZED \ - if (!_initialized) throw vespalib::IllegalStateException( \ - "initialize() must always be called first in order to " \ - "trigger lazy initialization.", VESPA_STRLOC) - + if (!_initialized) { \ + LOG(error, "initialize() must always be called first in order to trigger lazy initialization."); \ + abort(); \ + } BucketIdListResult DummyPersistence::listBuckets(BucketSpace bucketSpace) const @@ -715,7 +707,7 @@ DummyPersistence::destroyIterator(IteratorId id, Context&) } void -DummyPersistence::createBucketAsync(const Bucket& b, Context&, OperationComplete::UP onComplete) +DummyPersistence::createBucketAsync(const Bucket& b, Context&, OperationComplete::UP onComplete) noexcept { DUMMYPERSISTENCE_VERIFY_INITIALIZED; LOG(debug, "createBucket(%s)", b.toString().c_str()); @@ -731,7 +723,7 @@ DummyPersistence::createBucketAsync(const Bucket& b, Context&, OperationComplete } void -DummyPersistence::deleteBucketAsync(const Bucket& b, Context&, OperationComplete::UP onComplete) +DummyPersistence::deleteBucketAsync(const Bucket& b, Context&, OperationComplete::UP onComplete) noexcept { DUMMYPERSISTENCE_VERIFY_INITIALIZED; LOG(debug, "deleteBucket(%s)", b.toString().c_str()); diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h index 2ab97b0b403..a25bf6b8a8e 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h @@ -168,8 +168,8 @@ public: IterateResult iterate(IteratorId, uint64_t maxByteSize, Context&) const override; Result destroyIterator(IteratorId, Context&) override; - void createBucketAsync(const Bucket&, Context&, OperationComplete::UP) override; - void deleteBucketAsync(const Bucket&, Context&, OperationComplete::UP) override; + void createBucketAsync(const Bucket&, Context&, OperationComplete::UP) noexcept override; + void deleteBucketAsync(const Bucket&, Context&, OperationComplete::UP) noexcept override; Result split(const Bucket& source, const Bucket& target1, const Bucket& target2, Context&) override; diff --git a/persistence/src/vespa/persistence/spi/catchresult.h b/persistence/src/vespa/persistence/spi/catchresult.h index 02c626ea23e..7b04498205d 100644 --- a/persistence/src/vespa/persistence/spi/catchresult.h +++ b/persistence/src/vespa/persistence/spi/catchresult.h @@ -19,4 +19,9 @@ private: const ResultHandler *_resulthandler; }; +class NoopOperationComplete : public OperationComplete { + void onComplete(std::unique_ptr<spi::Result>) noexcept override { } + void addResultHandler(const spi::ResultHandler *) override { } +}; + } diff --git a/persistence/src/vespa/persistence/spi/persistenceprovider.h b/persistence/src/vespa/persistence/spi/persistenceprovider.h index 09a752d4ded..269175f7d26 100644 --- a/persistence/src/vespa/persistence/spi/persistenceprovider.h +++ b/persistence/src/vespa/persistence/spi/persistenceprovider.h @@ -337,14 +337,14 @@ struct PersistenceProvider * Tells the provider that the given bucket has been created in the * service layer. There is no requirement to do anything here. */ - virtual void createBucketAsync(const Bucket&, Context&, OperationComplete::UP) = 0; + virtual void createBucketAsync(const Bucket&, Context&, OperationComplete::UP) noexcept = 0; /** * Deletes the given bucket and all entries contained in that bucket. * After this operation has succeeded, a restart of the provider should * not yield the bucket in getBucketList(). */ - virtual void deleteBucketAsync(const Bucket&, Context&, OperationComplete::UP) = 0; + virtual void deleteBucketAsync(const Bucket&, Context&, OperationComplete::UP) noexcept = 0; /** * This function is called continuously by the service layer. It allows the diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp index 952c2218140..2e1fc74037c 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp @@ -549,7 +549,7 @@ PersistenceEngine::destroyIterator(IteratorId id, Context&) void -PersistenceEngine::createBucketAsync(const Bucket &b, Context &, OperationComplete::UP onComplete) +PersistenceEngine::createBucketAsync(const Bucket &b, Context &, OperationComplete::UP onComplete) noexcept { ReadGuard rguard(_rwMutex); LOG(spam, "createBucket(%s)", b.toString().c_str()); @@ -569,7 +569,7 @@ PersistenceEngine::createBucketAsync(const Bucket &b, Context &, OperationComple void -PersistenceEngine::deleteBucketAsync(const Bucket& b, Context&, OperationComplete::UP onComplete) +PersistenceEngine::deleteBucketAsync(const Bucket& b, Context&, OperationComplete::UP onComplete) noexcept { ReadGuard rguard(_rwMutex); LOG(spam, "deleteBucket(%s)", b.toString().c_str()); diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h index 9cabebc8135..fe564d01459 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h @@ -114,8 +114,8 @@ public: IterateResult iterate(IteratorId, uint64_t maxByteSize, Context&) const override; Result destroyIterator(IteratorId, Context&) override; - void createBucketAsync(const Bucket &bucketId, Context &, OperationComplete::UP) override; - void deleteBucketAsync(const Bucket&, Context&, OperationComplete::UP) override; + void createBucketAsync(const Bucket &bucketId, Context &, OperationComplete::UP) noexcept override; + void deleteBucketAsync(const Bucket&, Context&, OperationComplete::UP) noexcept override; BucketIdListResult getModifiedBuckets(BucketSpace bucketSpace) const override; Result split(const Bucket& source, const Bucket& target1, const Bucket& target2, Context&) override; Result join(const Bucket& source1, const Bucket& source2, const Bucket& target, Context&) override; diff --git a/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp b/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp index 302e603a9d8..02b43a32df3 100644 --- a/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp +++ b/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp @@ -24,7 +24,7 @@ #define CHECK_ERROR_ASYNC(className, failType, onError) \ { \ - Guard guard(_lock); \ + Guard guard(_lock); \ if (_result.getErrorCode() != spi::Result::ErrorType::NONE && (_failureMask & (failType))) { \ onError->onComplete(std::make_unique<className>(_result.getErrorCode(), _result.getErrorMessage())); \ return; \ @@ -81,7 +81,7 @@ PersistenceProviderWrapper::listBuckets(BucketSpace bucketSpace) const } void -PersistenceProviderWrapper::createBucketAsync(const spi::Bucket& bucket, spi::Context& context, spi::OperationComplete::UP onComplete) +PersistenceProviderWrapper::createBucketAsync(const spi::Bucket& bucket, spi::Context& context, spi::OperationComplete::UP onComplete) noexcept { LOG_SPI("createBucket(" << bucket << ")"); CHECK_ERROR_ASYNC(spi::Result, FAIL_CREATE_BUCKET, onComplete); @@ -177,7 +177,7 @@ PersistenceProviderWrapper::destroyIterator(spi::IteratorId iterId, void PersistenceProviderWrapper::deleteBucketAsync(const spi::Bucket& bucket, spi::Context& context, - spi::OperationComplete::UP operationComplete) + spi::OperationComplete::UP operationComplete) noexcept { LOG_SPI("deleteBucket(" << bucket << ")"); CHECK_ERROR_ASYNC(spi::Result, FAIL_DELETE_BUCKET, operationComplete); diff --git a/storage/src/tests/persistence/common/persistenceproviderwrapper.h b/storage/src/tests/persistence/common/persistenceproviderwrapper.h index 3cb7b92356b..cfc7002a643 100644 --- a/storage/src/tests/persistence/common/persistenceproviderwrapper.h +++ b/storage/src/tests/persistence/common/persistenceproviderwrapper.h @@ -96,7 +96,7 @@ public: void setActiveStateAsync(const spi::Bucket &bucket, spi::BucketInfo::ActiveState state, spi::OperationComplete::UP up) override; - void createBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) override; + void createBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) noexcept override; spi::BucketIdListResult listBuckets(BucketSpace bucketSpace) const override; spi::BucketInfoResult getBucketInfo(const spi::Bucket&) const override; void putAsync(const spi::Bucket&, spi::Timestamp, spi::DocumentSP, spi::Context&, spi::OperationComplete::UP) override; @@ -111,7 +111,7 @@ public: spi::IterateResult iterate(spi::IteratorId, uint64_t maxByteSize, spi::Context&) const override; spi::Result destroyIterator(spi::IteratorId, spi::Context&) override; - void deleteBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) override; + void deleteBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) noexcept override; spi::Result split(const spi::Bucket& source, const spi::Bucket& target1, const spi::Bucket& target2, spi::Context&) override; spi::Result join(const spi::Bucket& source1, const spi::Bucket& source2, diff --git a/storage/src/tests/persistence/filestorage/operationabortingtest.cpp b/storage/src/tests/persistence/filestorage/operationabortingtest.cpp index 75d9b595c4f..a3f0182ba30 100644 --- a/storage/src/tests/persistence/filestorage/operationabortingtest.cpp +++ b/storage/src/tests/persistence/filestorage/operationabortingtest.cpp @@ -62,13 +62,13 @@ public: return PersistenceProviderWrapper::getBucketInfo(bucket); } - void createBucketAsync(const spi::Bucket& bucket, spi::Context& ctx, spi::OperationComplete::UP onComplete) override { + void createBucketAsync(const spi::Bucket& bucket, spi::Context& ctx, spi::OperationComplete::UP onComplete) noexcept override { ++_createBucketInvocations; PersistenceProviderWrapper::createBucketAsync(bucket, ctx, std::move(onComplete)); } void - deleteBucketAsync(const spi::Bucket& bucket, spi::Context& ctx, spi::OperationComplete::UP onComplete) override { + deleteBucketAsync(const spi::Bucket& bucket, spi::Context& ctx, spi::OperationComplete::UP onComplete) noexcept override { ++_deleteBucketInvocations; PersistenceProviderWrapper::deleteBucketAsync(bucket, ctx, std::move(onComplete)); } diff --git a/storage/src/tests/persistence/mergehandlertest.cpp b/storage/src/tests/persistence/mergehandlertest.cpp index 017b8ce2b92..75e85fb4b6f 100644 --- a/storage/src/tests/persistence/mergehandlertest.cpp +++ b/storage/src/tests/persistence/mergehandlertest.cpp @@ -872,7 +872,6 @@ TEST_P(MergeHandlerTest, merge_bucket_spi_failures) { setUpChain(MIDDLE); ExpectedExceptionSpec exceptions[] = { - { PersistenceProviderWrapper::FAIL_CREATE_BUCKET, "create bucket" }, { PersistenceProviderWrapper::FAIL_BUCKET_INFO, "get bucket info" }, { PersistenceProviderWrapper::FAIL_CREATE_ITERATOR, "create iterator" }, { PersistenceProviderWrapper::FAIL_ITERATE, "iterate" }, @@ -903,7 +902,6 @@ TEST_P(MergeHandlerTest, get_bucket_diff_spi_failures) { setUpChain(MIDDLE); ExpectedExceptionSpec exceptions[] = { - { PersistenceProviderWrapper::FAIL_CREATE_BUCKET, "create bucket" }, { PersistenceProviderWrapper::FAIL_BUCKET_INFO, "get bucket info" }, { PersistenceProviderWrapper::FAIL_CREATE_ITERATOR, "create iterator" }, { PersistenceProviderWrapper::FAIL_ITERATE, "iterate" }, diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp index 5da2b65b7ff..bc6e67578c0 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.cpp +++ b/storage/src/vespa/storage/persistence/asynchandler.cpp @@ -5,6 +5,7 @@ #include "testandsethelper.h" #include "bucketownershipnotifier.h" #include <vespa/persistence/spi/persistenceprovider.h> +#include <vespa/persistence/spi/catchresult.h> #include <vespa/storageapi/message/bucket.h> #include <vespa/document/update/documentupdate.h> #include <vespa/vespalib/util/isequencedtaskexecutor.h> @@ -91,11 +92,6 @@ private: vespalib::ISequencedTaskExecutor::ExecutorId _executorId; }; -struct Noop : public spi::OperationComplete { - void onComplete(std::unique_ptr<spi::Result>) override { } - void addResultHandler(const spi::ResultHandler *) override { } -}; - bool bucketStatesAreSemanticallyEqual(const api::BucketInfo& a, const api::BucketInfo& b) { // Don't check document sizes, as background moving of documents in Proton @@ -174,7 +170,7 @@ AsyncHandler::handleCreateBucket(api::CreateBucketCommand& cmd, MessageTracker:: }); if (cmd.getActive()) { - _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<Noop>()); + _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<spi::NoopOperationComplete>()); _spi.setActiveStateAsync(bucket, spi::BucketInfo::ACTIVE, std::make_unique<ResultTaskOperationDone>(_sequencedExecutor, bucket, std::move(task))); } else { _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<ResultTaskOperationDone>(_sequencedExecutor, bucket, std::move(task))); diff --git a/storage/src/vespa/storage/persistence/mergehandler.cpp b/storage/src/vespa/storage/persistence/mergehandler.cpp index 7727a591af7..34466e527a0 100644 --- a/storage/src/vespa/storage/persistence/mergehandler.cpp +++ b/storage/src/vespa/storage/persistence/mergehandler.cpp @@ -6,6 +6,7 @@ #include "apply_bucket_diff_state.h" #include <vespa/storage/persistence/filestorage/mergestatus.h> #include <vespa/persistence/spi/persistenceprovider.h> +#include <vespa/persistence/spi/catchresult.h> #include <vespa/vespalib/stllike/asciistream.h> #include <vespa/vdslib/distribution/distribution.h> #include <vespa/document/fieldset/fieldsets.h> @@ -682,16 +683,6 @@ findCandidates(MergeStatus& status, uint16_t active_nodes_mask, bool constrictHa } } -struct CheckResult : public spi::OperationComplete { - spi::Bucket _bucket; - const char *_msg; - CheckResult(spi::Bucket bucket, const char * msg) : _bucket(bucket), _msg(msg) { } - void onComplete(std::unique_ptr<spi::Result> result) override { - checkResult(*result, _bucket, _msg); - } - void addResultHandler(const spi::ResultHandler *) override { } -}; - } api::StorageReply::SP @@ -910,7 +901,7 @@ MergeHandler::handleMergeBucket(api::MergeBucketCommand& cmd, MessageTracker::UP tracker->fail(api::ReturnCode::BUSY, err); return tracker; } - _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<CheckResult>(bucket, "create bucket")); + _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<spi::NoopOperationComplete>()); MergeStateDeleter stateGuard(_env._fileStorHandler, bucket.getBucket()); @@ -1073,7 +1064,7 @@ MergeHandler::handleGetBucketDiff(api::GetBucketDiffCommand& cmd, MessageTracker tracker->setMetric(_env._metrics.getBucketDiff); spi::Bucket bucket(cmd.getBucket()); LOG(debug, "GetBucketDiff(%s)", bucket.toString().c_str()); - _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<CheckResult>(bucket, "create bucket")); + _spi.createBucketAsync(bucket, tracker->context(), std::make_unique<spi::NoopOperationComplete>()); return handleGetBucketDiffStage2(cmd, std::move(tracker)); } diff --git a/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp b/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp index 5c27d112a7c..9ccd901744b 100644 --- a/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp +++ b/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp @@ -100,14 +100,14 @@ ProviderErrorWrapper::destroyIterator(spi::IteratorId iteratorId, spi::Context& } void -ProviderErrorWrapper::deleteBucketAsync(const spi::Bucket& bucket, spi::Context& context, spi::OperationComplete::UP onComplete) +ProviderErrorWrapper::deleteBucketAsync(const spi::Bucket& bucket, spi::Context& context, spi::OperationComplete::UP onComplete) noexcept { onComplete->addResultHandler(this); _impl.deleteBucketAsync(bucket, context, std::move(onComplete)); } void -ProviderErrorWrapper::createBucketAsync(const spi::Bucket& bucket, spi::Context& context, spi::OperationComplete::UP onComplete) +ProviderErrorWrapper::createBucketAsync(const spi::Bucket& bucket, spi::Context& context, spi::OperationComplete::UP onComplete) noexcept { onComplete->addResultHandler(this); _impl.createBucketAsync(bucket, context, std::move(onComplete)); diff --git a/storage/src/vespa/storage/persistence/provider_error_wrapper.h b/storage/src/vespa/storage/persistence/provider_error_wrapper.h index e999f33f2bd..14d20cf8a52 100644 --- a/storage/src/vespa/storage/persistence/provider_error_wrapper.h +++ b/storage/src/vespa/storage/persistence/provider_error_wrapper.h @@ -62,8 +62,8 @@ public: void removeIfFoundAsync(const spi::Bucket&, spi::Timestamp, const document::DocumentId&, spi::Context&, spi::OperationComplete::UP) override; void updateAsync(const spi::Bucket &, spi::Timestamp, spi::DocumentUpdateSP, spi::Context &, spi::OperationComplete::UP) override; void setActiveStateAsync(const spi::Bucket& b, spi::BucketInfo::ActiveState newState, spi::OperationComplete::UP onComplete) override; - void createBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) override; - void deleteBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) override; + void createBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) noexcept override; + void deleteBucketAsync(const spi::Bucket&, spi::Context&, spi::OperationComplete::UP) noexcept override; std::unique_ptr<vespalib::IDestructorCallback> register_executor(std::shared_ptr<spi::BucketExecutor> executor) override; private: template <typename ResultType> |