diff options
author | Tor Brede Vekterli <vekterli@verizonmedia.com> | 2020-10-16 15:06:28 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-10-16 15:06:28 +0200 |
commit | 702f3dab71f8d7e628cd6cd6b653ecfa72d4df66 (patch) | |
tree | 14cfc670a47074421ff68fe2d0c3d95dc68e1ada | |
parent | acb9d3ef78c4c61bde54357a5820a8ec88824b3a (diff) | |
parent | 08e49539165ac2893002102c84395166e70dd727 (diff) |
Merge pull request #14917 from vespa-engine/vekterli/simplify-bucket-db-persistence-provider-bootstrap-procedure
Greatly simplify bucket DB persistence provider bootstrap procedure
28 files changed, 235 insertions, 108 deletions
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp index b7acde31c4f..a58adec5011 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp @@ -359,6 +359,21 @@ DummyPersistence::setModifiedBuckets(const BucketIdListResult::List& buckets) _modifiedBuckets = buckets; } +void DummyPersistence::set_fake_bucket_set(const std::vector<std::pair<Bucket, BucketInfo>>& fake_info) { + std::lock_guard lock(_monitor); + _content.clear(); + for (auto& info : fake_info) { + const auto& bucket = info.first; + // DummyPersistence currently only supports default bucket space + assert(bucket.getBucketSpace() == FixedBucketSpaces::default_space()); + auto bucket_content = std::make_shared<BucketContent>(); + bucket_content->getMutableBucketInfo() = info.second; + // Must tag as up to date, or bucket info will be recomputed implicitly from zero state in getBucketInfo + bucket_content->setOutdatedInfo(false); + _content[bucket] = std::move(bucket_content); + } +} + BucketIdListResult DummyPersistence::getModifiedBuckets(BucketSpace bucketSpace) const { diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h index 5d49127a937..a7827b2c218 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h @@ -144,6 +144,10 @@ public: void setModifiedBuckets(const BucketIdListResult::List& result); + // Important: any subsequent mutations to the bucket set in fake_info will reset + // the bucket info due to implicit recalculation of bucket info. + void set_fake_bucket_set(const std::vector<std::pair<Bucket, BucketInfo>>& fake_info); + /** * Returns the list set by setModifiedBuckets(), then clears * the list. diff --git a/storage/src/tests/bucketdb/bucketmanagertest.cpp b/storage/src/tests/bucketdb/bucketmanagertest.cpp index 7c241f63a39..4513d00c903 100644 --- a/storage/src/tests/bucketdb/bucketmanagertest.cpp +++ b/storage/src/tests/bucketdb/bucketmanagertest.cpp @@ -164,7 +164,7 @@ void BucketManagerTest::setupTestEnvironment(bool fakePersistenceLayer, } else { auto bottom = std::make_unique<FileStorManager>( config.getConfigId(), - _node->getPersistenceProvider(), _node->getComponentRegister()); + _node->getPersistenceProvider(), _node->getComponentRegister(), *_node); _filestorManager = bottom.get(); _top->push_back(std::move(bottom)); } diff --git a/storage/src/tests/bucketdb/initializertest.cpp b/storage/src/tests/bucketdb/initializertest.cpp index c5d30204def..a39be7910f1 100644 --- a/storage/src/tests/bucketdb/initializertest.cpp +++ b/storage/src/tests/bucketdb/initializertest.cpp @@ -145,7 +145,7 @@ std::map<PartitionId, DiskData> createMapFromBucketDatabase(StorBucketDatabase& db) { std::map<PartitionId, DiskData> result; BucketInfoLogger infoLogger(result); - db.for_each(std::ref(infoLogger), "createmap"); + db.acquire_read_guard()->for_each(std::ref(infoLogger)); return result; } // Create data we want to have in this test diff --git a/storage/src/tests/bucketdb/lockablemaptest.cpp b/storage/src/tests/bucketdb/lockablemaptest.cpp index 527c0a927b4..1ea10003de1 100644 --- a/storage/src/tests/bucketdb/lockablemaptest.cpp +++ b/storage/src/tests/bucketdb/lockablemaptest.cpp @@ -251,11 +251,11 @@ TYPED_TEST(LockableMapTest, iterating) { // Test that we can use functor with non-const function { NonConstProcessor<TypeParam> ncproc; - map.for_each_mutable(std::ref(ncproc), "foo"); // First round of mutating functor for `all` + map.for_each_mutable_unordered(std::ref(ncproc), "foo"); // First round of mutating functor for `all` EXPECT_EQ(A(4, 7, 0), *map.get(11, "foo")); EXPECT_EQ(A(42,1, 0), *map.get(14, "foo")); EXPECT_EQ(A(1, 3, 3), *map.get(16, "foo")); - map.for_each_mutable(std::ref(ncproc), "foo"); // Once more, with feeling. + map.for_each_mutable_unordered(std::ref(ncproc), "foo"); // Once more, with feeling. EXPECT_EQ(A(4, 8, 0), *map.get(11, "foo")); EXPECT_EQ(A(42,2, 0), *map.get(14, "foo")); EXPECT_EQ(A(1, 4, 3), *map.get(16, "foo")); @@ -272,27 +272,13 @@ TYPED_TEST(LockableMapTest, iterating) { // Test that we can use const functors directly.. map.for_each(ConstProcessor<TypeParam>(), "foo"); - // Test iterator bounds - { - EntryProcessor<TypeParam> proc; - map.for_each_mutable(std::ref(proc), "foo", 11, 16); - std::string expected("11 - A(4, 8, 0)\n" - "14 - A(42, 2, 0)\n" - "16 - A(1, 4, 3)\n"); - EXPECT_EQ(expected, proc.toString()); - - EntryProcessor<TypeParam> proc2; - map.for_each_mutable(std::ref(proc2), "foo", 12, 15); - expected = "14 - A(42, 2, 0)\n"; - EXPECT_EQ(expected, proc2.toString()); - } - // Test that we can abort iterating + // Test that we can abort iterating { std::vector<typename TypeParam::Decision> decisions; decisions.push_back(TypeParam::CONTINUE); decisions.push_back(TypeParam::ABORT); EntryProcessor<TypeParam> proc(decisions); - map.for_each_mutable(std::ref(proc), "foo"); + map.for_each_mutable_unordered(std::ref(proc), "foo"); std::string expected("11 - A(4, 8, 0)\n" "14 - A(42, 2, 0)\n"); EXPECT_EQ(expected, proc.toString()); @@ -303,7 +289,7 @@ TYPED_TEST(LockableMapTest, iterating) { decisions.push_back(TypeParam::CONTINUE); decisions.push_back(TypeParam::REMOVE); // TODO consider removing; not used EntryProcessor<TypeParam> proc(decisions); - map.for_each_mutable(std::ref(proc), "foo"); + map.for_each_mutable_unordered(std::ref(proc), "foo"); std::string expected("11 - A(4, 8, 0)\n" "14 - A(42, 2, 0)\n" "16 - A(1, 4, 3)\n"); diff --git a/storage/src/tests/common/teststorageapp.h b/storage/src/tests/common/teststorageapp.h index 54668cac515..ffb16f3646a 100644 --- a/storage/src/tests/common/teststorageapp.h +++ b/storage/src/tests/common/teststorageapp.h @@ -43,7 +43,7 @@ DEFINE_PRIMITIVE_WRAPPER(uint16_t, Redundancy); class TestStorageApp : public framework::defaultimplementation::TestComponentRegister, - private DoneInitializeHandler + public DoneInitializeHandler { StorageComponentRegisterImpl& _compReg; @@ -100,10 +100,11 @@ public: private: // Storage server interface implementation (until we can remove it) - virtual api::Timestamp getUniqueTimestamp() { assert(0); throw; } - virtual StorBucketDatabase& getStorageBucketDatabase() { assert(0); throw; } - virtual BucketDatabase& getBucketDatabase() { assert(0); throw; } - virtual uint16_t getDiskCount() const { assert(0); throw; } + virtual api::Timestamp getUniqueTimestamp() { abort(); } + [[nodiscard]] virtual StorBucketDatabase& content_bucket_db(document::BucketSpace) { abort(); } + virtual StorBucketDatabase& getStorageBucketDatabase() { abort(); } + virtual BucketDatabase& getBucketDatabase() { abort(); } + virtual uint16_t getDiskCount() const { abort(); } }; class TestServiceLayerApp : public TestStorageApp @@ -125,6 +126,10 @@ public: spi::PersistenceProvider& getPersistenceProvider(); + StorBucketDatabase& content_bucket_db(document::BucketSpace space) override { + return _compReg.getBucketSpaceRepo().get(space).bucketDatabase(); + } + StorBucketDatabase& getStorageBucketDatabase() override { return _compReg.getBucketSpaceRepo().get(document::FixedBucketSpaces::default_space()).bucketDatabase(); } diff --git a/storage/src/tests/persistence/common/filestortestfixture.cpp b/storage/src/tests/persistence/common/filestortestfixture.cpp index 49dbf082e34..c6a0bdbb540 100644 --- a/storage/src/tests/persistence/common/filestortestfixture.cpp +++ b/storage/src/tests/persistence/common/filestortestfixture.cpp @@ -77,7 +77,8 @@ FileStorTestFixture::TestFileStorComponents::TestFileStorComponents( : _fixture(fixture), manager(new FileStorManager(fixture._config->getConfigId(), fixture._node->getPersistenceProvider(), - fixture._node->getComponentRegister())) + fixture._node->getComponentRegister(), + *fixture._node)) { injector.inject(top); top.push_back(StorageLink::UP(manager)); diff --git a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp index f61a31e5898..46e865a1ddd 100644 --- a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp +++ b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp @@ -112,8 +112,7 @@ struct FileStorManagerTest : Test{ } spi::dummy::DummyPersistence& getDummyPersistence() { - return static_cast<spi::dummy::DummyPersistence&> - (_node->getPersistenceProvider()); + return dynamic_cast<spi::dummy::DummyPersistence&>(_node->getPersistenceProvider()); } void setClusterState(const std::string& state) { @@ -213,7 +212,8 @@ struct TestFileStorComponents { explicit TestFileStorComponents(FileStorManagerTest& test) : manager(new FileStorManager(test.config->getConfigId(), test._node->getPersistenceProvider(), - test._node->getComponentRegister())) + test._node->getComponentRegister(), + *test._node)) { top.push_back(unique_ptr<StorageLink>(manager)); top.open(); @@ -239,7 +239,7 @@ TEST_F(FileStorManagerTest, header_only_put) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = - new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 3); // Creating a document to test with @@ -306,7 +306,7 @@ TEST_F(FileStorManagerTest, put) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = - new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 3); // Creating a document to test with @@ -339,7 +339,8 @@ TEST_F(FileStorManagerTest, state_change) { top.push_back(unique_ptr<StorageLink>(manager = new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), - _node->getComponentRegister()))); + _node->getComponentRegister(), + *_node))); top.open(); setClusterState("storage:3 distributor:3"); @@ -354,7 +355,7 @@ TEST_F(FileStorManagerTest, flush) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = new FileStorManager( - config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 3); // Creating a document to test with @@ -1267,7 +1268,7 @@ TEST_F(FileStorManagerTest, visiting) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = new FileStorManager( - smallConfig->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + smallConfig->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); // Adding documents to two buckets which we are going to visit // We want one bucket in one slotfile, and one bucket with a file split @@ -1385,7 +1386,7 @@ TEST_F(FileStorManagerTest, remove_location) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = - new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 3); document::BucketId bid(8, 0); @@ -1428,7 +1429,7 @@ TEST_F(FileStorManagerTest, delete_bucket) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = new FileStorManager( - config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 2); // Creating a document to test with @@ -1474,7 +1475,7 @@ TEST_F(FileStorManagerTest, delete_bucket_rejects_outdated_bucket_info) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = new FileStorManager( - config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 2); // Creating a document to test with @@ -1526,7 +1527,7 @@ TEST_F(FileStorManagerTest, delete_bucket_with_invalid_bucket_info){ DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = new FileStorManager( - config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 2); // Creating a document to test with @@ -1569,7 +1570,7 @@ TEST_F(FileStorManagerTest, no_timestamps) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = - new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address( "storage", lib::NodeType::STORAGE, 3); @@ -1613,7 +1614,7 @@ TEST_F(FileStorManagerTest, equal_timestamps) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = - new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 3); // Creating a document to test with @@ -1674,7 +1675,7 @@ TEST_F(FileStorManagerTest, get_iter) { DummyStorageLink top; FileStorManager *manager; top.push_back(unique_ptr<StorageLink>(manager = - new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); top.open(); api::StorageMessageAddress address( "storage", lib::NodeType::STORAGE, 3); @@ -1751,7 +1752,8 @@ TEST_F(FileStorManagerTest, set_bucket_active_state) { FileStorManager* manager( new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), - _node->getComponentRegister())); + _node->getComponentRegister(), + *_node)); top.push_back(unique_ptr<StorageLink>(manager)); setClusterState("storage:4 distributor:1"); top.open(); @@ -1829,7 +1831,8 @@ TEST_F(FileStorManagerTest, notify_owner_distributor_on_outdated_set_bucket_stat FileStorManager* manager( new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), - _node->getComponentRegister())); + _node->getComponentRegister(), + *_node)); top.push_back(unique_ptr<StorageLink>(manager)); setClusterState("storage:2 distributor:2"); @@ -1871,7 +1874,8 @@ TEST_F(FileStorManagerTest, GetBucketDiff_implicitly_creates_bucket) { FileStorManager* manager( new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), - _node->getComponentRegister())); + _node->getComponentRegister(), + *_node)); top.push_back(unique_ptr<StorageLink>(manager)); setClusterState("storage:2 distributor:1"); top.open(); @@ -1902,7 +1906,8 @@ TEST_F(FileStorManagerTest, merge_bucket_implicitly_creates_bucket) { FileStorManager* manager( new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), - _node->getComponentRegister())); + _node->getComponentRegister(), + *_node)); top.push_back(unique_ptr<StorageLink>(manager)); setClusterState("storage:3 distributor:1"); top.open(); @@ -1932,7 +1937,8 @@ TEST_F(FileStorManagerTest, newly_created_bucket_is_ready) { FileStorManager* manager( new FileStorManager(config->getConfigId(), _node->getPersistenceProvider(), - _node->getComponentRegister())); + _node->getComponentRegister(), + *_node)); top.push_back(unique_ptr<StorageLink>(manager)); setClusterState("storage:2 distributor:1"); top.open(); @@ -2057,4 +2063,58 @@ TEST_F(FileStorManagerTest, test_and_set_condition_mismatch_not_counted_as_failu EXPECT_EQ(thread_metrics_of(*c.manager)->failedOperations.getValue(), 0); } +namespace { + +spi::Bucket make_spi_bucket(uint32_t seed) { + return spi::Bucket(makeDocumentBucket(document::BucketId(15, seed))); +} + +spi::BucketInfo make_dummy_spi_bucket_info(uint32_t seed) { + return spi::BucketInfo(spi::BucketChecksum(seed + 0x1000), seed, seed * 100, seed, seed * 200); +} + +} + +TEST_F(FileStorManagerTest, bucket_db_is_populated_from_provider_when_initialize_is_called) { + TestFileStorComponents c(*this); + // TODO extend to test global bucket space as well. Dummy provider currently only + // supports default bucket space. Replace with a better mock. + std::vector<std::pair<spi::Bucket, spi::BucketInfo>> buckets = { + {make_spi_bucket(1), make_dummy_spi_bucket_info(1)}, + {make_spi_bucket(2), make_dummy_spi_bucket_info(2)}, + {make_spi_bucket(3), make_dummy_spi_bucket_info(3)}, + }; + std::sort(buckets.begin(), buckets.end(), [](auto& lhs, auto& rhs) { + return (lhs.first.getBucketId().toKey() < rhs.first.getBucketId().toKey()); + }); + + getDummyPersistence().set_fake_bucket_set(buckets); + c.manager->initialize_bucket_databases_from_provider(); + + std::vector<std::pair<document::BucketId, api::BucketInfo>> from_db; + auto populate_from_db = [&from_db](uint64_t key, auto& entry) { + from_db.emplace_back(document::BucketId::keyToBucketId(key), entry.info); + }; + + auto& default_db = _node->content_bucket_db(document::FixedBucketSpaces::default_space()); + default_db.acquire_read_guard()->for_each(populate_from_db); + ASSERT_EQ(from_db.size(), buckets.size()); + for (size_t i = 0; i < from_db.size(); ++i) { + auto& wanted = buckets[i]; + auto& actual = from_db[i]; + EXPECT_EQ(actual.first, wanted.first.getBucket().getBucketId()); + EXPECT_EQ(actual.second, PersistenceUtil::convertBucketInfo(wanted.second)); + } + + from_db.clear(); + auto& global_db = _node->content_bucket_db(document::FixedBucketSpaces::global_space()); + global_db.acquire_read_guard()->for_each(populate_from_db); + EXPECT_EQ(from_db.size(), 0); + + auto reported_state = _node->getStateUpdater().getReportedNodeState(); + EXPECT_EQ(reported_state->getMinUsedBits(), 15); + EXPECT_EQ(reported_state->getInitProgress(), 1.0); // Should be exact. + EXPECT_EQ(reported_state->getState(), lib::State::UP); +} + } // storage diff --git a/storage/src/tests/visiting/visitormanagertest.cpp b/storage/src/tests/visiting/visitormanagertest.cpp index 031290cecb4..ed54565c3c7 100644 --- a/storage/src/tests/visiting/visitormanagertest.cpp +++ b/storage/src/tests/visiting/visitormanagertest.cpp @@ -96,7 +96,7 @@ VisitorManagerTest::initializeTest() config.getConfigId(), _node->getComponentRegister(), *_messageSessionFactory))); _top->push_back(std::unique_ptr<StorageLink>(new FileStorManager( - config.getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister()))); + config.getConfigId(), _node->getPersistenceProvider(), _node->getComponentRegister(), *_node))); _manager->setTimeBetweenTicks(10); _top->open(); diff --git a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h index be83970e2b7..eedef2ec0ff 100644 --- a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h +++ b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h @@ -171,12 +171,10 @@ public: do_for_each_chunked(std::move(func), clientId, yieldTime, chunkSize); } - void for_each_mutable(std::function<Decision(uint64_t, ValueT&)> func, - const char* clientId, - const key_type& first = 0, - const key_type& last = UINT64_MAX) + void for_each_mutable_unordered(std::function<Decision(uint64_t, ValueT&)> func, + const char* clientId) { - do_for_each_mutable(std::move(func), clientId, first, last); + do_for_each_mutable_unordered(std::move(func), clientId); } void for_each(std::function<Decision(uint64_t, const ValueT&)> func, @@ -205,10 +203,8 @@ private: const char* clientId, vespalib::duration yieldTime, uint32_t chunkSize) = 0; - virtual void do_for_each_mutable(std::function<Decision(uint64_t, ValueT&)> func, - const char* clientId, - const key_type& first, - const key_type& last) = 0; + virtual void do_for_each_mutable_unordered(std::function<Decision(uint64_t, ValueT&)> func, + const char* clientId) = 0; virtual void do_for_each(std::function<Decision(uint64_t, const ValueT&)> func, const char* clientId, const key_type& first, diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h index 58e4564836b..ea3a7838d43 100644 --- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h +++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h @@ -117,10 +117,8 @@ private: bool handleDecision(key_type& key, mapped_type& val, Decision decision); void acquireKey(const LockId & lid, std::unique_lock<std::mutex> &guard); - void do_for_each_mutable(std::function<Decision(uint64_t, mapped_type&)> func, - const char* clientId, - const key_type& first, - const key_type& last) override; + void do_for_each_mutable_unordered(std::function<Decision(uint64_t, mapped_type&)> func, + const char* clientId) override; void do_for_each(std::function<Decision(uint64_t, const mapped_type&)> func, const char* clientId, diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp index c14afce1a7a..d291cfd371a 100644 --- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp +++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp @@ -273,16 +273,14 @@ bool BTreeLockableMap<T>::handleDecision(key_type& key, mapped_type& val, } template <typename T> -void BTreeLockableMap<T>::do_for_each_mutable(std::function<Decision(uint64_t, mapped_type&)> func, - const char* clientId, - const key_type& first, - const key_type& last) +void BTreeLockableMap<T>::do_for_each_mutable_unordered(std::function<Decision(uint64_t, mapped_type&)> func, + const char* clientId) { - key_type key = first; + key_type key = 0; mapped_type val; std::unique_lock guard(_lock); while (true) { - if (findNextKey(key, val, clientId, guard) || key > last) { + if (findNextKey(key, val, clientId, guard)) { return; } Decision d(func(key, val)); diff --git a/storage/src/vespa/storage/bucketdb/lockablemap.h b/storage/src/vespa/storage/bucketdb/lockablemap.h index 9584a23d84c..168439786e9 100644 --- a/storage/src/vespa/storage/bucketdb/lockablemap.h +++ b/storage/src/vespa/storage/bucketdb/lockablemap.h @@ -139,10 +139,8 @@ private: bool handleDecision(key_type& key, mapped_type& val, Decision decision); void acquireKey(const LockId & lid, std::unique_lock<std::mutex> &guard); - void do_for_each_mutable(std::function<Decision(uint64_t, mapped_type&)> func, - const char* clientId, - const key_type& first, - const key_type& last) override; + void do_for_each_mutable_unordered(std::function<Decision(uint64_t, mapped_type&)> func, + const char* clientId) override; void do_for_each(std::function<Decision(uint64_t, const mapped_type&)> func, const char* clientId, diff --git a/storage/src/vespa/storage/bucketdb/lockablemap.hpp b/storage/src/vespa/storage/bucketdb/lockablemap.hpp index 57183566964..f6ae420cb34 100644 --- a/storage/src/vespa/storage/bucketdb/lockablemap.hpp +++ b/storage/src/vespa/storage/bucketdb/lockablemap.hpp @@ -221,18 +221,20 @@ LockableMap<Map>::handleDecision(key_type& key, mapped_type& val, } template<typename Map> -void LockableMap<Map>::do_for_each_mutable(std::function<Decision(uint64_t, mapped_type&)> func, - const char* clientId, - const key_type& first, - const key_type& last) +void LockableMap<Map>::do_for_each_mutable_unordered(std::function<Decision(uint64_t, mapped_type&)> func, + const char* clientId) { - key_type key = first; + key_type key = 0; mapped_type val; std::unique_lock<std::mutex> guard(_lock); while (true) { - if (findNextKey(key, val, clientId, guard) || key > last) return; + if (findNextKey(key, val, clientId, guard)) { + return; + } Decision d(func(const_cast<const key_type&>(key), val)); - if (handleDecision(key, val, d)) return; + if (handleDecision(key, val, d)) { + return; + } ++key; } } @@ -247,10 +249,14 @@ void LockableMap<Map>::do_for_each(std::function<Decision(uint64_t, const mapped mapped_type val; std::unique_lock<std::mutex> guard(_lock); while (true) { - if (findNextKey(key, val, clientId, guard) || key > last) return; + if (findNextKey(key, val, clientId, guard) || key > last) { + return; + } Decision d(func(const_cast<const key_type&>(key), val)); assert(d == Decision::ABORT || d == Decision::CONTINUE); - if (handleDecision(key, val, d)) return; + if (handleDecision(key, val, d)) { + return; + } ++key; } } diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp index bb61867bcc5..9e692d7d602 100644 --- a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp +++ b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp @@ -121,13 +121,11 @@ void StorBucketDatabase::for_each_chunked( _impl->for_each_chunked(std::move(func), clientId, yieldTime, chunkSize); } -void StorBucketDatabase::for_each_mutable( +void StorBucketDatabase::for_each_mutable_unordered( std::function<Decision(uint64_t, bucketdb::StorageBucketInfo&)> func, - const char* clientId, - const key_type& first, - const key_type& last) + const char* clientId) { - _impl->for_each_mutable(std::move(func), clientId, first, last); + _impl->for_each_mutable_unordered(std::move(func), clientId); } void StorBucketDatabase::for_each( diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.h b/storage/src/vespa/storage/bucketdb/storbucketdb.h index ff86d7c9afa..61d8522ae9d 100644 --- a/storage/src/vespa/storage/bucketdb/storbucketdb.h +++ b/storage/src/vespa/storage/bucketdb/storbucketdb.h @@ -60,17 +60,15 @@ public: vespalib::duration yieldTime = 10us, uint32_t chunkSize = bucketdb::AbstractBucketMap<bucketdb::StorageBucketInfo>::DEFAULT_CHUNK_SIZE); - void for_each_mutable(std::function<Decision(uint64_t, bucketdb::StorageBucketInfo&)> func, - const char* clientId, - const key_type& first = key_type(), - const key_type& last = key_type() - 1); + void for_each_mutable_unordered(std::function<Decision(uint64_t, bucketdb::StorageBucketInfo&)> func, + const char* clientId); void for_each(std::function<Decision(uint64_t, const bucketdb::StorageBucketInfo&)> func, const char* clientId, const key_type& first = key_type(), const key_type& last = key_type() - 1); - std::unique_ptr<bucketdb::ReadGuard<Entry>> acquire_read_guard() const; + [[nodiscard]] std::unique_ptr<bucketdb::ReadGuard<Entry>> acquire_read_guard() const; /** * Returns true iff bucket has no superbuckets or sub-buckets in the diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp index 3b60e3c61ee..97d7feac5e9 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp @@ -3,9 +3,11 @@ #include "filestormanager.h" #include <vespa/storage/bucketdb/lockablemap.hpp> +#include <vespa/storage/bucketdb/minimumusedbitstracker.h> #include <vespa/storage/common/bucketmessages.h> #include <vespa/storage/common/bucketoperationlogger.h> #include <vespa/storage/common/content_bucket_space_repo.h> +#include <vespa/storage/common/doneinitializehandler.h> #include <vespa/vdslib/state/cluster_state_bundle.h> #include <vespa/storage/common/messagebucket.h> #include <vespa/storage/config/config-stor-server.h> @@ -26,8 +28,8 @@ using document::BucketSpace; namespace storage { FileStorManager:: -FileStorManager(const config::ConfigUri & configUri, - spi::PersistenceProvider& provider, ServiceLayerComponentRegister& compReg) +FileStorManager(const config::ConfigUri & configUri, spi::PersistenceProvider& provider, + ServiceLayerComponentRegister& compReg, DoneInitializeHandler& init_handler) : StorageLinkQueued("File store manager", compReg), framework::HtmlStatusReporter("filestorman", "File store manager"), _compReg(compReg), @@ -35,6 +37,7 @@ FileStorManager(const config::ConfigUri & configUri, _providerCore(provider), _providerErrorWrapper(_providerCore), _provider(&_providerErrorWrapper), + _init_handler(init_handler), _bucketIdFactory(_component.getBucketIdFactory()), _configUri(configUri), _threads(), @@ -223,11 +226,6 @@ FileStorManager::handlePersistenceMessage( const shared_ptr<api::StorageMessage> do { LOG(spam, "Received %s. Attempting to queue it.", msg->getType().getName().c_str()); - LOG_BUCKET_OPERATION_NO_LOCK( - getStorageMessageBucket(*msg).getBucketId(), - vespalib::make_string("Attempting to queue %s", msg->toString().c_str())); - - if (_filestorHandler->schedule(msg)) { LOG(spam, "Received persistence message %s. Queued it to disk", msg->getType().getName().c_str()); @@ -243,7 +241,7 @@ FileStorManager::handlePersistenceMessage( const shared_ptr<api::StorageMessage> case FileStorHandler::AVAILABLE: assert(false); } - } while(0); + } while (false); // If we get here, we failed to schedule message. errorCode says why // We need to reply to message (while not having bucket lock) if (!msg->getType().isReply()) { @@ -830,7 +828,7 @@ FileStorManager::updateState() if (contentBucketSpace.getNodeUpInLastNodeStateSeenByProvider() && !nodeUp) { LOG(debug, "Received cluster state where this node is down; de-activating all buckets in database for bucket space %s", bucketSpace.toString().c_str()); Deactivator deactivator; - contentBucketSpace.bucketDatabase().for_each_mutable( + contentBucketSpace.bucketDatabase().for_each_mutable_unordered( std::ref(deactivator), "FileStorManager::updateState"); } contentBucketSpace.setNodeUpInLastNodeStateSeenByProvider(nodeUp); @@ -862,4 +860,46 @@ FileStorManager::handleNewState() updateState(); } +void FileStorManager::update_reported_state_after_db_init() { + auto state_lock = _component.getStateUpdater().grabStateChangeLock(); + auto ns = *_component.getStateUpdater().getReportedNodeState(); + ns.setInitProgress(1.0); + ns.setMinUsedBits(_component.getMinUsedBitsTracker().getMinUsedBits()); + _component.getStateUpdater().setReportedNodeState(ns); +} + +void FileStorManager::initialize_bucket_databases_from_provider() { + framework::MilliSecTimer start_time(_component.getClock()); + size_t bucket_count = 0; + for (const auto& elem : _component.getBucketSpaceRepo()) { + const auto bucket_space = elem.first; + const auto bucket_result = _provider->listBuckets(bucket_space); + assert(!bucket_result.hasError()); + const auto& buckets = bucket_result.getList(); + LOG(debug, "Fetching bucket info for %zu buckets in space '%s'", + buckets.size(), elem.first.toString().c_str()); + auto& db = elem.second->bucketDatabase(); + + for (const auto& bucket : buckets) { + _component.getMinUsedBitsTracker().update(bucket); + // TODO replace with far more efficient bulk insert API + auto entry = db.get(bucket, "FileStorManager::initialize_bucket_databases_from_provider", + StorBucketDatabase::CREATE_IF_NONEXISTING); + assert(!entry.preExisted()); + auto spi_bucket = spi::Bucket(document::Bucket(bucket_space, bucket)); + auto provider_result = _provider->getBucketInfo(spi_bucket); + assert(!provider_result.hasError()); + entry->setBucketInfo(PersistenceUtil::convertBucketInfo(provider_result.getBucketInfo())); + entry.write(); + } + bucket_count += buckets.size(); + } + const double elapsed = start_time.getElapsedTimeAsDouble(); + LOG(info, "Completed listing of %zu buckets in %.2g milliseconds", bucket_count, elapsed); + _metrics->bucket_db_init_latency.addValue(elapsed); + + update_reported_state_after_db_init(); + _init_handler.notifyDoneInitializing(); +} + } // storage diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.h b/storage/src/vespa/storage/persistence/filestorage/filestormanager.h index 54bfd927b18..5e1aff18771 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.h +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.h @@ -40,6 +40,7 @@ struct FileStorManagerTest; class ReadBucketList; class BucketOwnershipNotifier; class AbortBucketOperationsCommand; +class DoneInitializeHandler; class FileStorManager : public StorageLinkQueued, public framework::HtmlStatusReporter, @@ -52,6 +53,7 @@ class FileStorManager : public StorageLinkQueued, spi::PersistenceProvider & _providerCore; ProviderErrorWrapper _providerErrorWrapper; spi::PersistenceProvider * _provider; + DoneInitializeHandler& _init_handler; const document::BucketIdFactory& _bucketIdFactory; config::ConfigUri _configUri; @@ -71,8 +73,8 @@ class FileStorManager : public StorageLinkQueued, friend struct FileStorManagerTest; public: - FileStorManager(const config::ConfigUri &, - spi::PersistenceProvider&, ServiceLayerComponentRegister&); + FileStorManager(const config::ConfigUri &, spi::PersistenceProvider&, + ServiceLayerComponentRegister&, DoneInitializeHandler&); FileStorManager(const FileStorManager &) = delete; FileStorManager& operator=(const FileStorManager &) = delete; @@ -93,6 +95,15 @@ public: void handleNewState() override; + // Must be called exactly once at startup _before_ storage chain is opened. + // This function expects that no external messages may arrive prior to, or + // concurrently with this call, such as client operations or cluster controller + // node state requests. + // By ensuring that this function is called prior to chain opening, this invariant + // shall be upheld since no RPC/MessageBus endpoints have been made available + // yet at that point in time. + void initialize_bucket_databases_from_provider(); + private: void configure(std::unique_ptr<vespa::config::content::StorFilestorConfig> config) override; @@ -152,6 +163,7 @@ private: void storageDistributionChanged() override; void updateState(); void propagateClusterStates(); + void update_reported_state_after_db_init(); }; } // storage diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp index 8e09291a507..6e0c474d9ba 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp @@ -255,7 +255,9 @@ FileStorMetrics::FileStorMetrics(const LoadTypeSet&) sum("alldisks", {{"sum"}}, "", this), directoryEvents("directoryevents", {}, "Number of directory events received.", this), partitionEvents("partitionevents", {}, "Number of partition events received.", this), - diskEvents("diskevents", {}, "Number of disk events received.", this) + diskEvents("diskevents", {}, "Number of disk events received.", this), + bucket_db_init_latency("bucket_db_init_latency", {}, "Time taken (in ms) to initialize bucket databases with " + "information from the persistence provider", this) { } FileStorMetrics::~FileStorMetrics() = default; diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h index 1ee47495502..ee82e17e7b3 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h +++ b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h @@ -164,6 +164,7 @@ struct FileStorMetrics : public metrics::MetricSet metrics::LongCountMetric directoryEvents; metrics::LongCountMetric partitionEvents; metrics::LongCountMetric diskEvents; + metrics::LongAverageMetric bucket_db_init_latency; explicit FileStorMetrics(const metrics::LoadTypeSet&); ~FileStorMetrics() override; diff --git a/storage/src/vespa/storage/persistence/persistencethread.cpp b/storage/src/vespa/storage/persistence/persistencethread.cpp index b6ce91c7ddb..cc238cd0146 100644 --- a/storage/src/vespa/storage/persistence/persistencethread.cpp +++ b/storage/src/vespa/storage/persistence/persistencethread.cpp @@ -161,7 +161,7 @@ PersistenceThread::checkProviderBucketInfoMatches(const spi::Bucket& bucket, con bucket.toString().c_str(), result.getErrorMessage().c_str()); return false; } - api::BucketInfo providerInfo(_env.convertBucketInfo(result.getBucketInfo())); + api::BucketInfo providerInfo(PersistenceUtil::convertBucketInfo(result.getBucketInfo())); // Don't check meta fields or active/ready fields since these are not // that important and ready may change under the hood in a race with // getModifiedBuckets(). If bucket is empty it means it has already diff --git a/storage/src/vespa/storage/persistence/persistenceutil.cpp b/storage/src/vespa/storage/persistence/persistenceutil.cpp index 1f1eb680b91..291db620723 100644 --- a/storage/src/vespa/storage/persistence/persistenceutil.cpp +++ b/storage/src/vespa/storage/persistence/persistenceutil.cpp @@ -248,7 +248,7 @@ PersistenceUtil::getBucketInfo(const document::Bucket &bucket) const } api::BucketInfo -PersistenceUtil::convertBucketInfo(const spi::BucketInfo& info) const +PersistenceUtil::convertBucketInfo(const spi::BucketInfo& info) { return api::BucketInfo(info.getChecksum(), info.getDocumentCount(), diff --git a/storage/src/vespa/storage/persistence/persistenceutil.h b/storage/src/vespa/storage/persistence/persistenceutil.h index bceaf544a9e..535bf379a89 100644 --- a/storage/src/vespa/storage/persistence/persistenceutil.h +++ b/storage/src/vespa/storage/persistence/persistenceutil.h @@ -135,7 +135,7 @@ struct PersistenceUtil { api::BucketInfo getBucketInfo(const document::Bucket &bucket) const; - api::BucketInfo convertBucketInfo(const spi::BucketInfo&) const; + static api::BucketInfo convertBucketInfo(const spi::BucketInfo&); void setBucketInfo(MessageTracker& tracker, const document::Bucket &bucket); diff --git a/storage/src/vespa/storage/storageserver/distributornode.h b/storage/src/vespa/storage/storageserver/distributornode.h index 2a5149aa8ac..17636efc7d3 100644 --- a/storage/src/vespa/storage/storageserver/distributornode.h +++ b/storage/src/vespa/storage/storageserver/distributornode.h @@ -52,6 +52,7 @@ public: private: void initializeNodeSpecific() override; + void perform_post_chain_creation_init_steps() override { /* no-op */ } void createChain(IStorageChainBuilder &builder) override; api::Timestamp getUniqueTimestamp() override; diff --git a/storage/src/vespa/storage/storageserver/servicelayernode.cpp b/storage/src/vespa/storage/storageserver/servicelayernode.cpp index 1916ae46510..6c48add6f53 100644 --- a/storage/src/vespa/storage/storageserver/servicelayernode.cpp +++ b/storage/src/vespa/storage/storageserver/servicelayernode.cpp @@ -215,13 +215,12 @@ ServiceLayerNode::createChain(IStorageChainBuilder &builder) auto merge_throttler = merge_throttler_up.get(); builder.add(std::move(merge_throttler_up)); builder.add(std::make_unique<ChangedBucketOwnershipHandler>(_configUri, compReg)); - builder.add(std::make_unique<StorageBucketDBInitializer>( - _configUri, getDoneInitializeHandler(), compReg)); builder.add(std::make_unique<BucketManager>(_configUri, _context.getComponentRegister())); builder.add(std::make_unique<VisitorManager>(_configUri, _context.getComponentRegister(), static_cast<VisitorMessageSessionFactory &>(*this), _externalVisitors)); builder.add(std::make_unique<ModifiedBucketChecker>( _context.getComponentRegister(), _persistenceProvider, _configUri)); - auto filstor_manager = std::make_unique<FileStorManager>(_configUri, _persistenceProvider, _context.getComponentRegister()); + auto filstor_manager = std::make_unique<FileStorManager>(_configUri, _persistenceProvider, _context.getComponentRegister(), + getDoneInitializeHandler()); _fileStorManager = filstor_manager.get(); builder.add(std::move(filstor_manager)); builder.add(releaseStateManager()); @@ -239,4 +238,9 @@ ServiceLayerNode::pause() return _fileStorManager->getFileStorHandler().pause(); } +void ServiceLayerNode::perform_post_chain_creation_init_steps() { + assert(_fileStorManager); + _fileStorManager->initialize_bucket_databases_from_provider(); +} + } // storage diff --git a/storage/src/vespa/storage/storageserver/servicelayernode.h b/storage/src/vespa/storage/storageserver/servicelayernode.h index 03603394e3a..9513888fd8d 100644 --- a/storage/src/vespa/storage/storageserver/servicelayernode.h +++ b/storage/src/vespa/storage/storageserver/servicelayernode.h @@ -58,6 +58,7 @@ public: private: void subscribeToConfigs() override; void initializeNodeSpecific() override; + void perform_post_chain_creation_init_steps() override; void handleLiveConfigUpdate(const InitialGuard & initGuard) override; VisitorMessageSession::UP createSession(Visitor&, VisitorThread&) override; documentapi::Priority::Value toDocumentPriority(uint8_t storagePriority) const override; diff --git a/storage/src/vespa/storage/storageserver/storagenode.cpp b/storage/src/vespa/storage/storageserver/storagenode.cpp index c19f8a7a11e..cb9d5730fa8 100644 --- a/storage/src/vespa/storage/storageserver/storagenode.cpp +++ b/storage/src/vespa/storage/storageserver/storagenode.cpp @@ -210,6 +210,8 @@ StorageNode::initialize() assert(_communicationManager != nullptr); _communicationManager->updateBucketSpacesConfig(*_bucketSpacesConfig); + perform_post_chain_creation_init_steps(); + // Start the metric manager, such that it starts generating snapshots // and the like. Note that at this time, all metrics should hopefully // have been created, such that we don't need to pay the extra cost of diff --git a/storage/src/vespa/storage/storageserver/storagenode.h b/storage/src/vespa/storage/storageserver/storagenode.h index adc55266588..a0997c4bacd 100644 --- a/storage/src/vespa/storage/storageserver/storagenode.h +++ b/storage/src/vespa/storage/storageserver/storagenode.h @@ -181,6 +181,7 @@ protected: void initialize(); virtual void subscribeToConfigs(); virtual void initializeNodeSpecific() = 0; + virtual void perform_post_chain_creation_init_steps() = 0; virtual void createChain(IStorageChainBuilder &builder) = 0; virtual void handleLiveConfigUpdate(const InitialGuard & initGuard); void shutdown(); |