diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2024-01-30 13:10:41 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2024-01-30 13:10:41 +0000 |
commit | 556ad57eab8cc2c93b38821a10c51a14345a1181 (patch) | |
tree | d19ecb55c576c098e249748b1a5b63df8ed9c347 /storage/src | |
parent | 568f8d94f3f7120e941ad0ac96b96da1b362ec9f (diff) |
GC leftovers from use_per_document_throttled_delete_bucket
Diffstat (limited to 'storage/src')
6 files changed, 41 insertions, 94 deletions
diff --git a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp index 4911ad88692..4846c90397a 100644 --- a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp +++ b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp @@ -5,6 +5,7 @@ #include <tests/common/teststorageapp.h> #include <tests/persistence/filestorage/forwardingmessagesender.h> #include <vespa/config/common/exceptions.h> +#include <memory> #include <vespa/config/helper/configgetter.hpp> #include <vespa/document/fieldset/fieldsets.h> #include <vespa/document/repo/documenttyperepo.h> @@ -92,16 +93,16 @@ struct FileStorTestBase : Test { std::unique_ptr<vdstestlib::DirConfig> config; std::unique_ptr<vdstestlib::DirConfig> config2; std::unique_ptr<vdstestlib::DirConfig> smallConfig; - const uint32_t _waitTime; + const int32_t _waitTime; const document::DocumentType* _testdoctype1; - FileStorTestBase() : _node(), _waitTime(LONG_WAITTIME) {} + FileStorTestBase(); ~FileStorTestBase() override; void SetUp() override; void TearDown() override; - void createBucket(document::BucketId bid) { + void createBucket(document::BucketId bid) const { _node->getPersistenceProvider().createBucket(makeSpiBucket(bid)); StorBucketDatabase::WrappedEntry entry( @@ -110,13 +111,13 @@ struct FileStorTestBase : Test { entry.write(); } - document::Document::UP createDocument(const std::string& content, const std::string& id) { + document::Document::UP createDocument(const std::string& content, const std::string& id) const { return _node->getTestDocMan().createDocument(content, id); } std::shared_ptr<api::PutCommand> make_put_command(StorageMessage::Priority pri = 20, const std::string& docid = "id:foo:testdoctype1::bar", - Timestamp timestamp = 100) { + Timestamp timestamp = 100) const { Document::SP doc(createDocument("my content", docid)); auto bucket = make_bucket_for_doc(doc->getId()); auto cmd = std::make_shared<api::PutCommand>(bucket, std::move(doc), timestamp); @@ -124,7 +125,7 @@ struct FileStorTestBase : Test { return cmd; } - std::shared_ptr<api::GetCommand> make_get_command(StorageMessage::Priority pri, + static std::shared_ptr<api::GetCommand> make_get_command(StorageMessage::Priority pri, const std::string& docid = "id:foo:testdoctype1::bar") { document::DocumentId did(docid); auto bucket = make_bucket_for_doc(did); @@ -139,12 +140,11 @@ struct FileStorTestBase : Test { auto clusterStateBundle = _node->getStateUpdater().getClusterStateBundle(); const auto &clusterState = *clusterStateBundle->getBaselineClusterState(); uint16_t distributor( - _node->getDistribution()->getIdealDistributorNode( - clusterState, bucket)); + _node->getDistribution()->getIdealDistributorNode(clusterState, bucket)); return distributor == distributorIndex; } - document::BucketId getFirstBucketNotOwnedByDistributor(uint16_t distributor) { + document::BucketId getFirstBucketNotOwnedByDistributor(uint16_t distributor) const { for (int i = 0; i < 1000; ++i) { if (!ownsBucket(distributor, document::BucketId(16, i))) { return document::BucketId(16, i); @@ -153,28 +153,25 @@ struct FileStorTestBase : Test { return document::BucketId(0); } - spi::dummy::DummyPersistence& getDummyPersistence() { + spi::dummy::DummyPersistence& getDummyPersistence() const { return dynamic_cast<spi::dummy::DummyPersistence&>(_node->getPersistenceProvider()); } - void setClusterState(const std::string& state) { - _node->getStateUpdater().setClusterState( - lib::ClusterState::CSP( - new lib::ClusterState(state))); + void setClusterState(const std::string& state) const { + _node->getStateUpdater().setClusterState(lib::ClusterState::CSP(new lib::ClusterState(state))); } void setupDisks() { std::string rootOfRoot = "filestormanagertest"; - config.reset(new vdstestlib::DirConfig(getStandardConfig(true, rootOfRoot))); + config = std::make_unique<vdstestlib::DirConfig>(getStandardConfig(true, rootOfRoot)); - config2.reset(new vdstestlib::DirConfig(*config)); + config2 = std::make_unique<vdstestlib::DirConfig>(*config); config2->getConfig("stor-server").set("root_folder", rootOfRoot + "-vdsroot.2"); config2->getConfig("stor-devices").set("root_folder", rootOfRoot + "-vdsroot.2"); config2->getConfig("stor-server").set("node_index", "1"); - smallConfig.reset(new vdstestlib::DirConfig(*config)); - vdstestlib::DirConfig::Config& c( - smallConfig->getConfig("stor-filestor", true)); + smallConfig = std::make_unique<vdstestlib::DirConfig>(*config); + vdstestlib::DirConfig::Config& c(smallConfig->getConfig("stor-filestor", true)); c.set("initial_index_read", "128"); c.set("use_direct_io", "false"); c.set("maximum_gap_to_read_through", "64"); @@ -202,11 +199,16 @@ struct FileStorTestBase : Test { std::shared_ptr<api::StorageMessage> cmd, const Metric& metric); - auto& thread_metrics_of(FileStorManager& manager) { + static auto& thread_metrics_of(FileStorManager& manager) { return manager.get_metrics().threads[0]; } }; +FileStorTestBase::FileStorTestBase() + : _node(), + _waitTime(LONG_WAITTIME), + _testdoctype1(nullptr) +{} FileStorTestBase::~FileStorTestBase() = default; std::unique_ptr<DiskThread> @@ -243,7 +245,8 @@ struct FileStorHandlerComponents { FileStorMetrics metrics; std::unique_ptr<FileStorHandler> filestorHandler; - FileStorHandlerComponents(FileStorTestBase& test, uint32_t threadsPerDisk = 1) + explicit FileStorHandlerComponents(FileStorTestBase& test) : FileStorHandlerComponents(test, 1) {} + FileStorHandlerComponents(FileStorTestBase& test, uint32_t threadsPerDisk) : top(), dummyManager(new DummyStorageLink), messageSender(*dummyManager), @@ -269,7 +272,7 @@ struct PersistenceHandlerComponents : public FileStorHandlerComponents { BucketOwnershipNotifier bucketOwnershipNotifier; std::unique_ptr<PersistenceHandler> persistenceHandler; - PersistenceHandlerComponents(FileStorTestBase& test) + explicit PersistenceHandlerComponents(FileStorTestBase& test) : FileStorHandlerComponents(test), executor(test._node->executor()), component(test._node->getComponentRegister(), "test"), @@ -311,7 +314,6 @@ FileStorTestBase::TearDown() } struct FileStorManagerTest : public FileStorTestBase { - void do_test_delete_bucket(bool use_throttled_delete); }; TEST_F(FileStorManagerTest, header_only_put) { @@ -767,8 +769,8 @@ TEST_F(FileStorManagerTest, priority) { document::BucketIdFactory factory; // Create buckets in separate, initial pass to avoid races with puts - for (uint32_t i=0; i<documents.size(); ++i) { - document::BucketId bucket(16, factory.getBucketId(documents[i]->getId()).getRawId()); + for (const auto & document : documents) { + document::BucketId bucket(16, factory.getBucketId(document->getId()).getRawId()); _node->getPersistenceProvider().createBucket(makeSpiBucket(bucket)); } @@ -980,9 +982,9 @@ TEST_F(FileStorManagerTest, split_single_group) { } // Test that the documents are all still there - for (uint32_t i=0; i<documents.size(); ++i) { + for (const auto & document : documents) { document::BucketId bucket(17, state ? 0x10001 : 0x00001); - auto cmd = std::make_shared<api::GetCommand>(makeDocumentBucket(bucket), documents[i]->getId(), document::AllFields::NAME); + auto cmd = std::make_shared<api::GetCommand>(makeDocumentBucket(bucket), document->getId(), document::AllFields::NAME); cmd->setAddress(_storage3); filestorHandler.schedule(cmd); filestorHandler.flush(true); @@ -1159,8 +1161,8 @@ TEST_F(FileStorManagerTest, join) { // Perform a join, check that other files are gone { auto cmd = std::make_shared<api::JoinBucketsCommand>(makeDocumentBucket(document::BucketId(16, 1))); - cmd->getSourceBuckets().emplace_back(document::BucketId(17, 0x00001)); - cmd->getSourceBuckets().emplace_back(document::BucketId(17, 0x10001)); + cmd->getSourceBuckets().emplace_back(17, 0x00001); + cmd->getSourceBuckets().emplace_back(17, 0x10001); filestorHandler.schedule(cmd); filestorHandler.flush(true); ASSERT_EQ(1, top.getNumReplies()); @@ -1371,12 +1373,11 @@ TEST_F(FileStorManagerTest, remove_location) { } } -void FileStorManagerTest::do_test_delete_bucket(bool use_throttled_delete) { +TEST_F(FileStorManagerTest, delete_bucket) { TestFileStorComponents c(*this); auto config_uri = config::ConfigUri(config->getConfigId()); StorFilestorConfigBuilder my_config(*config_from<StorFilestorConfig>(config_uri)); - my_config.usePerDocumentThrottledDeleteBucket = use_throttled_delete; c.manager->on_configure(my_config); auto& top = c.top; @@ -1421,23 +1422,12 @@ void FileStorManagerTest::do_test_delete_bucket(bool use_throttled_delete) { StorBucketDatabase::WrappedEntry entry(_node->getStorageBucketDatabase().get(bid, "foo")); EXPECT_FALSE(entry.exists()); } - if (use_throttled_delete) { - auto& metrics = thread_metrics_of(*c.manager)->remove_by_gid; - EXPECT_EQ(metrics.failed.getValue(), 0); - EXPECT_EQ(metrics.count.getValue(), 1); - // We can't reliably test the actual latency here without wiring mock clock bumping into - // the async remove by GID execution, but we can at least test that we updated the metric. - EXPECT_EQ(metrics.latency.getCount(), 1); - } -} - -// TODO remove once throttled behavior is the default -TEST_F(FileStorManagerTest, delete_bucket_legacy) { - do_test_delete_bucket(false); -} - -TEST_F(FileStorManagerTest, delete_bucket_throttled) { - do_test_delete_bucket(true); + auto& metrics = thread_metrics_of(*c.manager)->remove_by_gid; + EXPECT_EQ(metrics.failed.getValue(), 0); + EXPECT_EQ(metrics.count.getValue(), 1); + // We can't reliably test the actual latency here without wiring mock clock bumping into + // the async remove by GID execution, but we can at least test that we updated the metric. + EXPECT_EQ(metrics.latency.getCount(), 1); } TEST_F(FileStorManagerTest, delete_bucket_rejects_outdated_bucket_info) { diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp index a69f9e55afb..725cf2c7511 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.cpp +++ b/storage/src/vespa/storage/persistence/asynchandler.cpp @@ -225,29 +225,6 @@ AsyncHandler::on_delete_bucket_complete(const document::Bucket& bucket) const { } } -MessageTracker::UP -AsyncHandler::handleDeleteBucket(api::DeleteBucketCommand& cmd, MessageTracker::UP tracker) const -{ - tracker->setMetric(_env._metrics.deleteBuckets); - LOG(debug, "DeletingBucket(%s)", cmd.getBucketId().toString().c_str()); - if (_env._fileStorHandler.isMerging(cmd.getBucket())) { - _env._fileStorHandler.clearMergeStatus(cmd.getBucket(), - api::ReturnCode(api::ReturnCode::ABORTED, "Bucket was deleted during the merge")); - } - spi::Bucket bucket(cmd.getBucket()); - if (!checkProviderBucketInfoMatches(bucket, cmd.getBucketInfo())) { - return tracker; - } - - auto task = makeResultTask([this, tracker = std::move(tracker), bucket = cmd.getBucket()]([[maybe_unused]] spi::Result::UP ignored) { - // TODO Even if an non OK response can not be handled sanely we might probably log a message, or increment a metric - on_delete_bucket_complete(bucket); - tracker->sendReply(); - }); - _spi.deleteBucketAsync(bucket, std::make_unique<ResultTaskOperationDone>(_sequencedExecutor, cmd.getBucketId(), std::move(task))); - return tracker; -} - namespace { class GatherBucketMetadata : public BucketProcessor::EntryProcessor { diff --git a/storage/src/vespa/storage/persistence/asynchandler.h b/storage/src/vespa/storage/persistence/asynchandler.h index 1433176036b..c78dfe6282d 100644 --- a/storage/src/vespa/storage/persistence/asynchandler.h +++ b/storage/src/vespa/storage/persistence/asynchandler.h @@ -31,7 +31,6 @@ public: MessageTrackerUP handleUpdate(api::UpdateCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleRunTask(RunTaskCommand & cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleSetBucketState(api::SetBucketStateCommand& cmd, MessageTrackerUP tracker) const; - MessageTrackerUP handleDeleteBucket(api::DeleteBucketCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handle_delete_bucket_throttling(api::DeleteBucketCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleCreateBucket(api::CreateBucketCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleRemoveLocation(api::RemoveLocationCommand& cmd, MessageTrackerUP tracker) const; diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp index 61c7da6e286..ecf5b33200b 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp @@ -248,7 +248,6 @@ FileStorManager::on_configure(const StorFilestorConfig& config) std::lock_guard guard(_lock); for (auto& ph : _persistenceHandlers) { ph->set_throttle_merge_feed_ops(throttle_merge_feed_ops); - ph->set_use_per_document_throttled_delete_bucket(config.usePerDocumentThrottledDeleteBucket); } } } diff --git a/storage/src/vespa/storage/persistence/persistencehandler.cpp b/storage/src/vespa/storage/persistence/persistencehandler.cpp index 78ad6eac0e2..29d39845f5a 100644 --- a/storage/src/vespa/storage/persistence/persistencehandler.cpp +++ b/storage/src/vespa/storage/persistence/persistencehandler.cpp @@ -24,8 +24,7 @@ PersistenceHandler::PersistenceHandler(vespalib::ISequencedTaskExecutor & sequen cfg.commonMergeChainOptimalizationMinimumSize), _asyncHandler(_env, provider, bucketOwnershipNotifier, sequencedExecutor, component.getBucketIdFactory()), _splitJoinHandler(_env, provider, bucketOwnershipNotifier, cfg.enableMultibitSplitOptimalization), - _simpleHandler(_env, provider, component.getBucketIdFactory()), - _use_per_op_throttled_delete_bucket(false) + _simpleHandler(_env, provider, component.getBucketIdFactory()) { } @@ -69,11 +68,7 @@ PersistenceHandler::handleCommandSplitByType(api::StorageCommand& msg, MessageTr case api::MessageType::CREATEBUCKET_ID: return _asyncHandler.handleCreateBucket(static_cast<api::CreateBucketCommand&>(msg), std::move(tracker)); case api::MessageType::DELETEBUCKET_ID: - if (use_per_op_throttled_delete_bucket()) { - return _asyncHandler.handle_delete_bucket_throttling(static_cast<api::DeleteBucketCommand&>(msg), std::move(tracker)); - } else { - return _asyncHandler.handleDeleteBucket(static_cast<api::DeleteBucketCommand&>(msg), std::move(tracker)); - } + return _asyncHandler.handle_delete_bucket_throttling(static_cast<api::DeleteBucketCommand&>(msg), std::move(tracker)); case api::MessageType::JOINBUCKETS_ID: return _splitJoinHandler.handleJoinBuckets(static_cast<api::JoinBucketsCommand&>(msg), std::move(tracker)); case api::MessageType::SPLITBUCKET_ID: @@ -114,7 +109,7 @@ PersistenceHandler::handleCommandSplitByType(api::StorageCommand& msg, MessageTr default: break; } - return MessageTracker::UP(); + return {}; } MessageTracker::UP @@ -186,14 +181,4 @@ PersistenceHandler::set_throttle_merge_feed_ops(bool throttle) noexcept _mergeHandler.set_throttle_merge_feed_ops(throttle); } -bool -PersistenceHandler::use_per_op_throttled_delete_bucket() const noexcept { - return _use_per_op_throttled_delete_bucket.load(std::memory_order_relaxed); -} - -void -PersistenceHandler::set_use_per_document_throttled_delete_bucket(bool throttle) noexcept { - _use_per_op_throttled_delete_bucket.store(throttle, std::memory_order_relaxed); -} - } diff --git a/storage/src/vespa/storage/persistence/persistencehandler.h b/storage/src/vespa/storage/persistence/persistencehandler.h index 9639b772a28..595815d2bb3 100644 --- a/storage/src/vespa/storage/persistence/persistencehandler.h +++ b/storage/src/vespa/storage/persistence/persistencehandler.h @@ -38,14 +38,12 @@ public: const SimpleMessageHandler & simpleMessageHandler() const { return _simpleHandler; } void set_throttle_merge_feed_ops(bool throttle) noexcept; - void set_use_per_document_throttled_delete_bucket(bool throttle) noexcept; private: // Message handling functions MessageTracker::UP handleCommandSplitByType(api::StorageCommand&, MessageTracker::UP tracker) const; MessageTracker::UP handleReply(api::StorageReply&, MessageTracker::UP) const; MessageTracker::UP processMessage(api::StorageMessage& msg, MessageTracker::UP tracker) const; - [[nodiscard]] bool use_per_op_throttled_delete_bucket() const noexcept; const framework::Clock & _clock; PersistenceUtil _env; @@ -54,7 +52,6 @@ private: AsyncHandler _asyncHandler; SplitJoinHandler _splitJoinHandler; SimpleMessageHandler _simpleHandler; - std::atomic<bool> _use_per_op_throttled_delete_bucket; }; } // storage |