diff options
Diffstat (limited to 'storage/src/tests')
35 files changed, 289 insertions, 137 deletions
diff --git a/storage/src/tests/bucketdb/bucketmanagertest.cpp b/storage/src/tests/bucketdb/bucketmanagertest.cpp index f41dae89eec..dc560cf58cd 100644 --- a/storage/src/tests/bucketdb/bucketmanagertest.cpp +++ b/storage/src/tests/bucketdb/bucketmanagertest.cpp @@ -1,9 +1,10 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include <tests/common/dummystoragelink.h> +#include <tests/common/storage_config_set.h> #include <tests/common/testhelper.h> #include <tests/common/teststorageapp.h> -#include <tests/common/storage_config_set.h> +#include <vespa/config-stor-distribution.h> #include <vespa/config/helper/configgetter.hpp> #include <vespa/document/config/config-documenttypes.h> #include <vespa/document/datatype/documenttype.h> @@ -25,7 +26,6 @@ #include <vespa/vespalib/gtest/gtest.h> #include <vespa/vespalib/stllike/asciistream.h> #include <vespa/vespalib/testkit/test_path.h> -#include <vespa/config-stor-distribution.h> #include <future> #include <vespa/log/log.h> @@ -116,16 +116,17 @@ public: BucketManagerTest::~BucketManagerTest() = default; -#define ASSERT_DUMMYLINK_REPLY_COUNT(link, count) \ - if (link->getNumReplies() != count) { \ - std::ostringstream ost; \ - ost << "Expected there to be " << count << " replies in link, but " \ - << "found " << link->getNumReplies() << ":\n"; \ - for (uint32_t i=0; i<link->getNumReplies(); ++i) { \ - ost << link->getReply(i)->getType() << "\n"; \ - } \ - FAIL() << ost.str(); \ +void check_dummy_link_reply_count(const DummyStorageLink& link, size_t expected_count) { + if (link.getNumReplies() != expected_count) { + std::ostringstream ost; + ost << "Expected there to be " << expected_count << " replies in link, but " + << "found " << link.getNumReplies() << ":\n"; + for (uint32_t i = 0; i < link.getNumReplies(); ++i) { + ost << link.getReply(i)->getType() << "\n"; + } + FAIL() << ost.str(); } +} void BucketManagerTest::setupTestEnvironment() { @@ -313,7 +314,7 @@ TEST_F(BucketManagerTest, DISABLED_request_bucket_info_with_state) { { LOG(info, "Waiting for response from 3 request bucket info messages"); _top->waitForMessages(3, 5); - ASSERT_DUMMYLINK_REPLY_COUNT(_top, 3); + ASSERT_NO_FATAL_FAILURE(check_dummy_link_reply_count(*_top, 3)); std::map<uint64_t, api::RequestBucketInfoReply::SP> replies; for (uint32_t i=0; i<3; ++i) { replies[_top->getReply(i)->getMsgId()] @@ -357,7 +358,7 @@ TEST_F(BucketManagerTest, request_bucket_info_with_list) { _top->sendDown(cmd); _top->waitForMessages(1, 5); - ASSERT_DUMMYLINK_REPLY_COUNT(_top, 1); + ASSERT_NO_FATAL_FAILURE(check_dummy_link_reply_count(*_top, 1)); auto reply = std::dynamic_pointer_cast<api::RequestBucketInfoReply>(_top->getReply(0)); _top->reset(); ASSERT_TRUE(reply.get()); @@ -527,7 +528,7 @@ class ConcurrentOperationFixture { public: explicit ConcurrentOperationFixture(BucketManagerTest& self) : _self(self), - _state(std::make_shared<lib::ClusterState>("distributor:1 storage:1")) + _state(std::make_shared<lib::ClusterState>("version:2 distributor:1 storage:1")) { _self.setupTestEnvironment(); _self._top->open(); @@ -601,7 +602,7 @@ public: return std::make_shared<api::RequestBucketInfoCommand>(makeBucketSpace(), 0, explicit_state); } - auto createFullFetchCommandWithHash(vespalib::stringref hash) const { + auto createFullFetchCommandWithHash(std::string_view hash) const { return std::make_shared<api::RequestBucketInfoCommand>(makeBucketSpace(), 0, *_state, hash); } diff --git a/storage/src/tests/common/hostreporter/hostinfotest.cpp b/storage/src/tests/common/hostreporter/hostinfotest.cpp index 13d53007af4..9b4e6e6c4c5 100644 --- a/storage/src/tests/common/hostreporter/hostinfotest.cpp +++ b/storage/src/tests/common/hostreporter/hostinfotest.cpp @@ -1,6 +1,5 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include "util.h" #include <vespa/storage/common/hostreporter/hostinfo.h> #include <vespa/storage/common/hostreporter/hostreporter.h> #include <vespa/vespalib/data/slime/slime.h> @@ -38,7 +37,7 @@ TEST(HostInfoReporterTest, host_info_reporter) { hostinfo.printReport(stream); stream << End(); - std::string jsonData = json.str(); + std::string_view jsonData = json.str(); vespalib::Slime slime; JsonFormat::decode(Memory(jsonData), slime); EXPECT_EQ(slime.get()["dummy"]["foo"].asString(), "bar"); diff --git a/storage/src/tests/common/hostreporter/util.cpp b/storage/src/tests/common/hostreporter/util.cpp index f87fb9728fb..65e3456601a 100644 --- a/storage/src/tests/common/hostreporter/util.cpp +++ b/storage/src/tests/common/hostreporter/util.cpp @@ -22,7 +22,7 @@ reporterToSlime(HostReporter &hostReporter, vespalib::Slime &slime) { stream << Object(); hostReporter.report(stream); stream << End(); - std::string jsonData = json.str(); + std::string jsonData(json.str()); size_t parsed = JsonFormat::decode(Memory(jsonData), slime); if (parsed == 0) { diff --git a/storage/src/tests/common/testnodestateupdater.cpp b/storage/src/tests/common/testnodestateupdater.cpp index f9671617352..5d1d7a085b9 100644 --- a/storage/src/tests/common/testnodestateupdater.cpp +++ b/storage/src/tests/common/testnodestateupdater.cpp @@ -25,17 +25,28 @@ TestNodeStateUpdater::getClusterStateBundle() const } void +TestNodeStateUpdater::patch_distribution(std::shared_ptr<const lib::Distribution> distribution) +{ + _clusterStateBundle = _clusterStateBundle->clone_with_new_distribution( + lib::DistributionConfigBundle::of(std::move(distribution))); +} + +void TestNodeStateUpdater::setClusterState(std::shared_ptr<const lib::ClusterState> c) { - setClusterStateBundle(std::make_shared<const lib::ClusterStateBundle>(*c)); + setClusterStateBundle(std::make_shared<const lib::ClusterStateBundle>(std::move(c))); } void TestNodeStateUpdater::setClusterStateBundle(std::shared_ptr<const lib::ClusterStateBundle> clusterStateBundle) { + auto existing_distr = _clusterStateBundle->distribution_config_bundle(); _clusterStateBundle = std::move(clusterStateBundle); - for (uint32_t i = 0; i < _listeners.size(); ++i) { - _listeners[i]->handleNewState(); + if (!_clusterStateBundle->has_distribution_config() && existing_distr) { + _clusterStateBundle = _clusterStateBundle->clone_with_new_distribution(existing_distr); + } + for (auto* listener : _listeners) { + listener->handleNewState(); } } diff --git a/storage/src/tests/common/testnodestateupdater.h b/storage/src/tests/common/testnodestateupdater.h index e5418c238d5..6a00e9a2264 100644 --- a/storage/src/tests/common/testnodestateupdater.h +++ b/storage/src/tests/common/testnodestateupdater.h @@ -1,11 +1,4 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -/** - * \class storage::TestNodeStateUpdater - * \ingroup common - * - * \brief Test implementation of the node state updater. - */ - #pragma once #include <vespa/storage/common/nodestateupdater.h> @@ -14,6 +7,7 @@ namespace storage::lib { class ClusterState; class ClusterStateBundle; + class Distribution; } namespace storage { @@ -53,6 +47,7 @@ public: _current = std::make_shared<lib::NodeState>(state); } + void patch_distribution(std::shared_ptr<const lib::Distribution> distribution); void setClusterState(std::shared_ptr<const lib::ClusterState> c); void setClusterStateBundle(std::shared_ptr<const lib::ClusterStateBundle> clusterStateBundle); diff --git a/storage/src/tests/common/teststorageapp.cpp b/storage/src/tests/common/teststorageapp.cpp index d811f100aec..b2b82a46850 100644 --- a/storage/src/tests/common/teststorageapp.cpp +++ b/storage/src/tests/common/teststorageapp.cpp @@ -6,10 +6,10 @@ #include <vespa/storage/config/config-stor-distributormanager.h> #include <vespa/storage/config/config-stor-visitordispatcher.h> #include <vespa/config-stor-distribution.h> -#include <vespa/config-fleetcontroller.h> #include <vespa/persistence/dummyimpl/dummypersistence.h> #include <vespa/vdslib/distribution/distribution.h> #include <vespa/vdslib/state/clusterstate.h> +#include <vespa/vdslib/state/cluster_state_bundle.h> #include <vespa/vespalib/util/exceptions.h> #include <vespa/vespalib/util/time.h> #include <vespa/config/subscription/configuri.h> @@ -59,6 +59,7 @@ TestStorageApp::TestStorageApp(StorageComponentRegisterImpl::UP compReg, auto distr = std::make_shared<lib::Distribution>( lib::Distribution::getDefaultDistributionConfig(redundancy, nodeCount)); _compReg.setDistribution(distr); + _nodeStateUpdater.patch_distribution(distr); } TestStorageApp::~TestStorageApp() = default; @@ -69,6 +70,7 @@ TestStorageApp::setDistribution(Redundancy redundancy, NodeCount nodeCount) auto distr = std::make_shared<lib::Distribution>( lib::Distribution::getDefaultDistributionConfig(redundancy, nodeCount)); _compReg.setDistribution(distr); + _nodeStateUpdater.patch_distribution(distr); } void @@ -83,6 +85,12 @@ TestStorageApp::setClusterState(const lib::ClusterState& c) _nodeStateUpdater.setClusterState(std::make_shared<lib::ClusterState>(c)); } +void +TestStorageApp::set_cluster_state_bundle(std::shared_ptr<const lib::ClusterStateBundle> state_bundle) +{ + _nodeStateUpdater.setClusterStateBundle(std::move(state_bundle)); +} + namespace { NodeIndex node_index_from_config(const config::ConfigUri& uri) { diff --git a/storage/src/tests/common/teststorageapp.h b/storage/src/tests/common/teststorageapp.h index 04fa6996e15..c423761a9a2 100644 --- a/storage/src/tests/common/teststorageapp.h +++ b/storage/src/tests/common/teststorageapp.h @@ -73,6 +73,7 @@ public: void setDistribution(Redundancy, NodeCount); void setTypeRepo(std::shared_ptr<const document::DocumentTypeRepo> repo); void setClusterState(const lib::ClusterState&); + void set_cluster_state_bundle(std::shared_ptr<const lib::ClusterStateBundle>); // Utility functions for getting a hold of currently used bits. Practical // to avoid adding extra components in the tests. @@ -81,7 +82,7 @@ public: std::shared_ptr<const document::DocumentTypeRepo> getTypeRepo() { return _compReg.getTypeRepo(); } const document::BucketIdFactory& getBucketIdFactory() { return _compReg.getBucketIdFactory(); } TestNodeStateUpdater& getStateUpdater() { return _nodeStateUpdater; } - std::shared_ptr<lib::Distribution> & getDistribution() { return _compReg.getDistribution(); } + std::shared_ptr<const lib::Distribution> getDistribution() { return _compReg.getDistribution(); } TestNodeStateUpdater& getNodeStateUpdater() { return _nodeStateUpdater; } uint16_t getIndex() const { return _compReg.getIndex(); } const NodeIdentity& node_identity() const noexcept { return _node_identity; } diff --git a/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp b/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp index c56911a066e..ce8ae2362aa 100644 --- a/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp +++ b/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp @@ -81,7 +81,7 @@ getBucketSpaceStats(const vespalib::Slime& root, uint16_t nodeIndex, const vespa { const auto& bucketSpaces = getNode(root, nodeIndex)["bucket-spaces"]; for (size_t i = 0; i < bucketSpaces.entries(); ++i) { - if (bucketSpaces[i]["name"].asString().make_stringref() == bucketSpaceName) { + if (bucketSpaces[i]["name"].asString().make_stringview() == bucketSpaceName) { return bucketSpaces[i]; } } @@ -181,7 +181,7 @@ TEST_F(DistributorHostInfoReporterTest, generate_example_json) { stream << End(); stream.finalize(); - std::string jsonString = json.str(); + std::string_view jsonString = json.str(); std::string path = TEST_PATH("../../../../protocols/getnodestate/distributor.json"); std::string goldenString = File::readAll(path); diff --git a/storage/src/tests/distributor/distributor_stripe_test.cpp b/storage/src/tests/distributor/distributor_stripe_test.cpp index fbc9cb44462..ee1e05154bb 100644 --- a/storage/src/tests/distributor/distributor_stripe_test.cpp +++ b/storage/src/tests/distributor/distributor_stripe_test.cpp @@ -670,7 +670,7 @@ TEST_F(DistributorStripeTest, external_client_requests_are_handled_individually_ std::vector<api::StorageMessage::Priority> priorities({50, 255, 10, 40, 0}); document::DocumentId id("id:foo:testdoctype1:n=1:foo"); - vespalib::stringref field_set = ""; + std::string_view field_set = ""; for (auto pri : priorities) { auto cmd = std::make_shared<api::GetCommand>(makeDocumentBucket(document::BucketId()), id, field_set); cmd->setPriority(pri); @@ -728,7 +728,7 @@ TEST_F(DistributorStripeTest, closing_aborts_priority_queued_client_requests) addNodesToBucketDB(bucket, "0=1/1/1/t"); document::DocumentId id("id:foo:testdoctype1:n=1:foo"); - vespalib::stringref field_set = ""; + std::string_view field_set = ""; for (int i = 0; i < 10; ++i) { auto cmd = std::make_shared<api::GetCommand>(makeDocumentBucket(document::BucketId()), id, field_set); _stripe->handle_or_enqueue_message(cmd); @@ -1071,10 +1071,23 @@ TEST_F(DistributorStripeTest, bucket_ownership_change_does_not_cancel_pending_op do_test_not_cancelled_pending_op_without_bucket_ownership_change(true); } -// TODO we do not have good handling of bucket ownership changes combined with -// distribution config changes... Hard to remove all such edge cases unless we -// make state+config change an atomic operation initiated by the cluster controller -// (hint: we should do this). +TEST_F(DistributorStripeTest, maintenance_operation_cancellation_does_not_invoke_recheck_with_invalid_bucket) { + setup_stripe(Redundancy(1), NodeCount(2), "version:1 distributor:1 storage:2"); + configure_enable_operation_cancellation(true); + addNodesToBucketDB(BucketId(16, 1), "0=3/4/5/t/u/r,1=4/5/6/t/u/r"); + + tickDistributorNTimes(10); // ==> Activation sent to node 1 (largest ready replica) + ASSERT_EQ(_sender.getCommands(true), "SetBucketState => 1"); + + // Node 1 takes a nosedive; the pending SetBucketState operation is cancelled internally. + simulate_cluster_state_transition("version:2 distributor:1 storage:2 .1.s:d", true); + auto reply = std::make_shared<api::SetBucketStateReply>(dynamic_cast<api::SetBucketStateCommand&>(*_sender.command(0))); + _stripe->handleReply(reply); + // If we have gotten this far without exploding with an invariant check failure, all is well. + EXPECT_EQ("BucketId(0x4000000000000001) : " + "node(idx=0,crc=0x3,docs=4/4,bytes=5/5,trusted=true,active=false,ready=true)", // no node 1 + dumpBucket(BucketId(16, 1))); +} } diff --git a/storage/src/tests/distributor/distributor_stripe_test_util.cpp b/storage/src/tests/distributor/distributor_stripe_test_util.cpp index 0628b62bdfe..9cc7934ddb6 100644 --- a/storage/src/tests/distributor/distributor_stripe_test_util.cpp +++ b/storage/src/tests/distributor/distributor_stripe_test_util.cpp @@ -537,7 +537,7 @@ DistributorStripeTestUtil::getBucketSpaces() const } void -DistributorStripeTestUtil::enable_cluster_state(vespalib::stringref state) +DistributorStripeTestUtil::enable_cluster_state(std::string_view state) { getBucketDBUpdater().simulate_cluster_state_bundle_activation(lib::ClusterStateBundle(lib::ClusterState(state))); } diff --git a/storage/src/tests/distributor/distributor_stripe_test_util.h b/storage/src/tests/distributor/distributor_stripe_test_util.h index 862d9bfbfba..d6877c0b3e0 100644 --- a/storage/src/tests/distributor/distributor_stripe_test_util.h +++ b/storage/src/tests/distributor/distributor_stripe_test_util.h @@ -264,7 +264,7 @@ protected: }; MessageSenderImpl _messageSender; - void enable_cluster_state(vespalib::stringref state); + void enable_cluster_state(std::string_view state); void enable_cluster_state(const lib::ClusterStateBundle& state); }; diff --git a/storage/src/tests/distributor/externaloperationhandlertest.cpp b/storage/src/tests/distributor/externaloperationhandlertest.cpp index 33da4727017..5677e5e6628 100644 --- a/storage/src/tests/distributor/externaloperationhandlertest.cpp +++ b/storage/src/tests/distributor/externaloperationhandlertest.cpp @@ -32,8 +32,8 @@ namespace storage::distributor { struct ExternalOperationHandlerTest : Test, DistributorStripeTestUtil { document::TestDocMan _testDocMan; - document::BucketId findNonOwnedUserBucketInState(vespalib::stringref state); - document::BucketId findOwned1stNotOwned2ndInStates(vespalib::stringref state1, vespalib::stringref state2); + document::BucketId findNonOwnedUserBucketInState(std::string_view state); + document::BucketId findOwned1stNotOwned2ndInStates(std::string_view state1, std::string_view state2); std::shared_ptr<api::GetCommand> makeGetCommandForUser(uint64_t id) const; std::shared_ptr<api::GetCommand> makeGetCommand(const vespalib::string& id) const; @@ -130,7 +130,7 @@ TEST_F(ExternalOperationHandlerTest, bucket_split_mask) { document::BucketId ExternalOperationHandlerTest::findNonOwnedUserBucketInState( - vespalib::stringref statestr) + std::string_view statestr) { lib::ClusterState state(statestr); for (uint64_t i = 1; i < 1000; ++i) { @@ -144,8 +144,8 @@ ExternalOperationHandlerTest::findNonOwnedUserBucketInState( document::BucketId ExternalOperationHandlerTest::findOwned1stNotOwned2ndInStates( - vespalib::stringref statestr1, - vespalib::stringref statestr2) + std::string_view statestr1, + std::string_view statestr2) { lib::ClusterState state1(statestr1); lib::ClusterState state2(statestr2); diff --git a/storage/src/tests/distributor/mergeoperationtest.cpp b/storage/src/tests/distributor/mergeoperationtest.cpp index 754974ebf97..19752e2db9e 100644 --- a/storage/src/tests/distributor/mergeoperationtest.cpp +++ b/storage/src/tests/distributor/mergeoperationtest.cpp @@ -148,13 +148,12 @@ TEST_F(MergeOperationTest, fail_if_delete_bucket_fails) { namespace { std::string getNodeList(std::string state, uint32_t redundancy, std::string existing) { - lib::Distribution distribution( - lib::Distribution::getDefaultDistributionConfig(redundancy)); + lib::Distribution distribution(lib::Distribution::getDefaultDistributionConfig(redundancy)); lib::ClusterState clusterState(state); vespalib::StringTokenizer st(existing, ","); std::vector<BucketCopy> bucketDB(st.size()); for (uint32_t i = 0; i < st.size(); i++) { - std::string num = st[i]; + std::string num(st[i]); size_t pos = num.find('t'); bool trusted = false; diff --git a/storage/src/tests/distributor/read_for_write_visitor_operation_test.cpp b/storage/src/tests/distributor/read_for_write_visitor_operation_test.cpp index 21116f8a281..d4b8c0605af 100644 --- a/storage/src/tests/distributor/read_for_write_visitor_operation_test.cpp +++ b/storage/src/tests/distributor/read_for_write_visitor_operation_test.cpp @@ -215,7 +215,7 @@ TEST_F(ReadForWriteVisitorOperationStarterTest, visitor_locks_bucket_with_random auto cmd = std::dynamic_pointer_cast<api::CreateVisitorCommand>(_sender.command(0)); ASSERT_TRUE(cmd); EXPECT_EQ(cmd->getParameters().get(reindexing_bucket_lock_visitor_parameter_key(), - vespalib::stringref("not found :I")), + std::string_view("not found :I")), "fritjof"); } diff --git a/storage/src/tests/distributor/simplemaintenancescannertest.cpp b/storage/src/tests/distributor/simplemaintenancescannertest.cpp index 3541857e029..8ed4a98168a 100644 --- a/storage/src/tests/distributor/simplemaintenancescannertest.cpp +++ b/storage/src/tests/distributor/simplemaintenancescannertest.cpp @@ -28,7 +28,7 @@ struct SimpleMaintenanceScannerTest : Test { void addBucketToDb(document::BucketSpace bucketSpace, int bucketNum); void addBucketToDb(int bucketNum); - bool scanEntireDatabase(int expected); + bool scanEntireDatabase(int expected) const; std::string stringifyGlobalPendingStats(const PendingStats& stats) const; void SetUp() override; @@ -58,8 +58,7 @@ SimpleMaintenanceScannerTest::addBucketToDb(int bucketNum) } std::string -SimpleMaintenanceScannerTest::stringifyGlobalPendingStats( - const PendingStats& stats) const +SimpleMaintenanceScannerTest::stringifyGlobalPendingStats(const PendingStats& stats) const { std::ostringstream ss; ss << stats.global; @@ -99,7 +98,7 @@ namespace { std::string sortLines(const std::string& source) { vespalib::StringTokenizer st(source,"\n",""); - std::vector<std::string> lines; + std::vector<vespalib::string> lines; std::copy(st.begin(), st.end(), std::back_inserter(lines)); std::sort(lines.begin(), lines.end()); std::ostringstream ost; @@ -125,7 +124,7 @@ TEST_F(SimpleMaintenanceScannerTest, prioritize_multiple_buckets) { } bool -SimpleMaintenanceScannerTest::scanEntireDatabase(int expected) +SimpleMaintenanceScannerTest::scanEntireDatabase(int expected) const { for (int i = 0; i < expected; ++i) { if (_scanner->scanNext().isDone()) { diff --git a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp index 4fe8d88fb8d..26242720966 100644 --- a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp +++ b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp @@ -407,10 +407,10 @@ public: std::vector<uint16_t> expand_node_vec(const std::vector<uint16_t>& nodes); void trigger_completed_but_not_yet_activated_transition( - vespalib::stringref initial_state_str, + std::string_view initial_state_str, uint32_t initial_buckets, uint32_t initial_expected_msgs, - vespalib::stringref pending_state_str, + std::string_view pending_state_str, uint32_t pending_buckets, uint32_t pending_expected_msgs); @@ -603,10 +603,10 @@ TopLevelBucketDBUpdaterTest::get_node_list(const std::vector<uint16_t>& nodes) void TopLevelBucketDBUpdaterTest::trigger_completed_but_not_yet_activated_transition( - vespalib::stringref initial_state_str, + std::string_view initial_state_str, uint32_t initial_buckets, uint32_t initial_expected_msgs, - vespalib::stringref pending_state_str, + std::string_view pending_state_str, uint32_t pending_buckets, uint32_t pending_expected_msgs) { diff --git a/storage/src/tests/distributor/top_level_distributor_test.cpp b/storage/src/tests/distributor/top_level_distributor_test.cpp index 8463e0b548c..4fa55ec5756 100644 --- a/storage/src/tests/distributor/top_level_distributor_test.cpp +++ b/storage/src/tests/distributor/top_level_distributor_test.cpp @@ -56,7 +56,6 @@ struct TopLevelDistributorTest : Test, TopLevelDistributorTestUtil { // added type safety. using NodeCount = int; using Redundancy = int; - using ConfigBuilder = vespa::config::content::core::StorDistributormanagerConfigBuilder; std::string resolve_stripe_operation_routing(const std::shared_ptr<api::StorageMessage> & msg) { handle_top_level_message(msg); @@ -70,7 +69,7 @@ struct TopLevelDistributorTest : Test, TopLevelDistributorTestUtil { } stripes[i]->_messageQueue.clear(); } - return posted_msgs.str(); + return std::string(posted_msgs.str()); } StatusReporterDelegate& distributor_status_delegate() { diff --git a/storage/src/tests/distributor/top_level_distributor_test_util.cpp b/storage/src/tests/distributor/top_level_distributor_test_util.cpp index 4816c1dce22..4fb3fba904b 100644 --- a/storage/src/tests/distributor/top_level_distributor_test_util.cpp +++ b/storage/src/tests/distributor/top_level_distributor_test_util.cpp @@ -419,7 +419,7 @@ TopLevelDistributorTestUtil::all_distributor_stripes_are_in_recovery_mode() cons } void -TopLevelDistributorTestUtil::enable_distributor_cluster_state(vespalib::stringref state, +TopLevelDistributorTestUtil::enable_distributor_cluster_state(std::string_view state, bool has_bucket_ownership_transfer) { bucket_db_updater().simulate_cluster_state_bundle_activation( diff --git a/storage/src/tests/distributor/top_level_distributor_test_util.h b/storage/src/tests/distributor/top_level_distributor_test_util.h index 5ae6646502f..b4fd0ccec12 100644 --- a/storage/src/tests/distributor/top_level_distributor_test_util.h +++ b/storage/src/tests/distributor/top_level_distributor_test_util.h @@ -168,7 +168,7 @@ protected: MessageSenderImpl _message_sender; uint32_t _num_distributor_stripes; - void enable_distributor_cluster_state(vespalib::stringref state, bool has_bucket_ownership_transfer = false); + void enable_distributor_cluster_state(std::string_view state, bool has_bucket_ownership_transfer = false); void enable_distributor_cluster_state(const lib::ClusterStateBundle& state); }; diff --git a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp index 8ccca37e6c1..02473f31a54 100644 --- a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp +++ b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp @@ -134,7 +134,7 @@ struct TwoPhaseUpdateOperationTest : Test, DistributorStripeTestUtil { _timestampToUpdate = ts; return *this; } - UpdateOptions& condition(vespalib::stringref cond) { + UpdateOptions& condition(std::string_view cond) { _condition = documentapi::TestAndSetCondition(cond); return *this; } diff --git a/storage/src/tests/distributor/visitoroperationtest.cpp b/storage/src/tests/distributor/visitoroperationtest.cpp index 5ad0ef939b0..cf07303fd24 100644 --- a/storage/src/tests/distributor/visitoroperationtest.cpp +++ b/storage/src/tests/distributor/visitoroperationtest.cpp @@ -1114,7 +1114,7 @@ TEST_F(VisitorOperationTest, assigning_put_lock_access_token_sets_special_visito auto cmd = std::dynamic_pointer_cast<api::CreateVisitorCommand>(_sender.command(0)); ASSERT_TRUE(cmd); EXPECT_EQ(cmd->getParameters().get(reindexing_bucket_lock_visitor_parameter_key(), - vespalib::stringref("")), + std::string_view("")), "its-a me, mario"); } diff --git a/storage/src/tests/frameworkimpl/status/statustest.cpp b/storage/src/tests/frameworkimpl/status/statustest.cpp index 8592a332f0c..72886d2c29d 100644 --- a/storage/src/tests/frameworkimpl/status/statustest.cpp +++ b/storage/src/tests/frameworkimpl/status/statustest.cpp @@ -30,7 +30,7 @@ vespalib::string fetch(int port, const vespalib::string &path) { vespalib::string result; ssize_t res = conn->read(buf, sizeof(buf)); while (res > 0) { - result.append(vespalib::stringref(buf, res)); + result.append(std::string_view(buf, res)); res = conn->read(buf, sizeof(buf)); } assert(res == 0); diff --git a/storage/src/tests/persistence/filestorage/feed_operation_batching_test.cpp b/storage/src/tests/persistence/filestorage/feed_operation_batching_test.cpp index cf16123933b..1e4f581b779 100644 --- a/storage/src/tests/persistence/filestorage/feed_operation_batching_test.cpp +++ b/storage/src/tests/persistence/filestorage/feed_operation_batching_test.cpp @@ -139,7 +139,7 @@ struct FeedOperationBatchingTest : FileStorTestFixture { auto id = as_cmd->getDocumentId(); ASSERT_TRUE(id.getScheme().hasNumber()); EXPECT_EQ(id.getScheme().getNumber(), expected_bucket_idx) << id; - std::string actual_id_part = id.getScheme().getNamespaceSpecific(); + std::string actual_id_part(id.getScheme().getNamespaceSpecific()); std::string expected_id_part = std::to_string(expected_doc_idx); EXPECT_EQ(actual_id_part, expected_id_part) << id; } diff --git a/storage/src/tests/persistence/mergehandlertest.cpp b/storage/src/tests/persistence/mergehandlertest.cpp index 524f5bae392..1a80374d182 100644 --- a/storage/src/tests/persistence/mergehandlertest.cpp +++ b/storage/src/tests/persistence/mergehandlertest.cpp @@ -44,7 +44,7 @@ struct MergeHandlerTest : PersistenceTestUtils { // @TODO Add test to test that buildBucketInfo and mergeLists create minimal list (wrong sorting screws this up) - void fillDummyApplyDiff(std::vector<api::ApplyBucketDiffCommand::Entry>& diff); + void fillDummyApplyDiff(std::vector<api::ApplyBucketDiffCommand::Entry>& diff) const; std::shared_ptr<api::ApplyBucketDiffCommand> createDummyApplyDiff( int timestampOffset, uint16_t hasMask = 0x1, @@ -73,7 +73,6 @@ struct MergeHandlerTest : PersistenceTestUtils { virtual void invoke(MergeHandlerTest&, MergeHandler&, spi::Context&) = 0; virtual std::string afterInvoke(MergeHandlerTest&, MergeHandler&) = 0; }; - friend class HandlerInvoker; class NoReplyHandlerInvoker : public HandlerInvoker @@ -92,13 +91,6 @@ struct MergeHandlerTest : PersistenceTestUtils { void invoke(MergeHandlerTest&, MergeHandler&, spi::Context&) override; }; - class HandleMergeBucketReplyInvoker - : public NoReplyHandlerInvoker - { - public: - void invoke(MergeHandlerTest&, MergeHandler&, spi::Context&) override; - }; - class HandleGetBucketDiffInvoker : public NoReplyHandlerInvoker { @@ -168,13 +160,13 @@ struct MergeHandlerTest : PersistenceTestUtils { HandlerInvoker& invoker, const ExpectedExceptionSpec& spec); - MergeHandler createHandler(size_t maxChunkSize = 0x400000) { - return MergeHandler(getEnv(), getPersistenceProvider(), - getEnv()._component.cluster_context(), getEnv()._component.getClock(), *_sequenceTaskExecutor, maxChunkSize); + MergeHandler createHandler(uint32_t maxChunkSize = 0x400000) { + return {getEnv(), getPersistenceProvider(), getEnv()._component.cluster_context(), + getEnv()._component.getClock(), *_sequenceTaskExecutor, maxChunkSize}; } MergeHandler createHandler(spi::PersistenceProvider & spi) { - return MergeHandler(getEnv(), spi, - getEnv()._component.cluster_context(), getEnv()._component.getClock(), *_sequenceTaskExecutor, 4190208); + return {getEnv(), spi, getEnv()._component.cluster_context(), + getEnv()._component.getClock(), *_sequenceTaskExecutor, 4190208}; } std::shared_ptr<api::StorageMessage> get_queued_reply() { @@ -425,8 +417,8 @@ size_t getFilledCount(const std::vector<api::ApplyBucketDiffCommand::Entry>& diff) { size_t filledCount = 0; - for (size_t i=0; i<diff.size(); ++i) { - if (diff[i].filled()) { + for (const auto & i : diff) { + if (i.filled()) { ++filledCount; } } @@ -437,9 +429,9 @@ size_t getFilledDataSize(const std::vector<api::ApplyBucketDiffCommand::Entry>& diff) { size_t filledSize = 0; - for (size_t i=0; i<diff.size(); ++i) { - filledSize += diff[i]._headerBlob.size(); - filledSize += diff[i]._bodyBlob.size(); + for (const auto & i : diff) { + filledSize += i._headerBlob.size(); + filledSize += i._bodyBlob.size(); } return filledSize; } @@ -480,14 +472,14 @@ TEST_F(MergeHandlerTest, chunked_apply_bucket_diff) { // Include node 1 in hasmask for all diffs to indicate it's done // Also remember the diffs we've seen thus far to ensure chunking // does not send duplicates. - for (size_t i = 0; i < diff.size(); ++i) { - if (!diff[i].filled()) { + for (auto & i : diff) { + if (!i.filled()) { continue; } - diff[i]._entry._hasMask |= 2u; - auto inserted = seen.emplace(spi::Timestamp(diff[i]._entry._timestamp)); + i._entry._hasMask |= 2u; + auto inserted = seen.emplace(i._entry._timestamp); if (!inserted.second) { - FAIL() << "Diff for " << diff[i] + FAIL() << "Diff for " << i << " has already been seen in another ApplyBucketDiff"; } } @@ -561,8 +553,7 @@ TEST_F(MergeHandlerTest, max_timestamp) { } void -MergeHandlerTest::fillDummyApplyDiff( - std::vector<api::ApplyBucketDiffCommand::Entry>& diff) +MergeHandlerTest::fillDummyApplyDiff(std::vector<api::ApplyBucketDiffCommand::Entry>& diff) const { document::TestDocMan docMan; document::Document::SP doc(docMan.createRandomDocumentAtLocation(_location)); @@ -776,7 +767,7 @@ MergeHandlerTest::convert_delayed_error_to_exception(MergeHandler& handler) getEnv()._fileStorHandler.clearMergeStatus(_bucket, return_code); fetchSingleMessage<api::ApplyBucketDiffReply>(); fetchSingleMessage<api::ApplyBucketDiffCommand>(); - throw std::runtime_error(return_code.getMessage()); + throw std::runtime_error(std::string(return_code.getMessage())); } } } @@ -1052,7 +1043,7 @@ MergeHandlerTest::HandleApplyBucketDiffReplyInvoker::convert_delayed_error_to_ex auto chained_reply = _stub.replies.back(); _stub.replies.pop_back(); test.messageKeeper().sendReply(chained_reply); - throw std::runtime_error(chained_reply->getResult().getMessage()); + throw std::runtime_error(std::string(chained_reply->getResult().getMessage())); } } @@ -1221,8 +1212,7 @@ TEST_F(MergeHandlerTest, remove_put_on_existing_timestamp) { // Timestamp should now be a regular remove bool foundTimestamp = false; - for (size_t i = 0; i < getBucketDiffCmd->getDiff().size(); ++i) { - const api::GetBucketDiffCommand::Entry& e(getBucketDiffCmd->getDiff()[i]); + for (const auto & e : getBucketDiffCmd->getDiff()) { if (e._timestamp == ts) { EXPECT_EQ( uint16_t(MergeHandler::IN_USE | MergeHandler::DELETED), @@ -1285,22 +1275,6 @@ std::ostream &operator<<(std::ostream &os, const EntryCheck &entry) } -namespace api { - -std::ostream &operator<<(std::ostream &os, const MergeBucketCommand::Node &node) -{ - os << "Node(" << node.index << "," << (node.sourceOnly ? "true" : "false") << ")"; - return os; -} - -std::ostream &operator<<(std::ostream &os, const GetBucketDiffCommand::Entry &entry) -{ - os << "Entry(timestamp=" << entry._timestamp << ", hasMask=" << entry._hasMask << ")"; - return os; -} - -} - TEST_F(MergeHandlerTest, partially_filled_apply_bucket_diff_reply) { using NodeList = decltype(_nodes); diff --git a/storage/src/tests/persistence/provider_error_wrapper_test.cpp b/storage/src/tests/persistence/provider_error_wrapper_test.cpp index d5ce8400b25..7ecc9dac30e 100644 --- a/storage/src/tests/persistence/provider_error_wrapper_test.cpp +++ b/storage/src/tests/persistence/provider_error_wrapper_test.cpp @@ -16,11 +16,11 @@ struct ProviderErrorWrapperTest : PersistenceTestUtils { namespace { struct MockErrorListener : ProviderErrorListener { - void on_fatal_error(vespalib::stringref message) override { + void on_fatal_error(std::string_view message) override { _seen_fatal_error = true; _fatal_error = message; } - void on_resource_exhaustion_error(vespalib::stringref message) override { + void on_resource_exhaustion_error(std::string_view message) override { _seen_resource_exhaustion_error = true; _resource_exhaustion_error = message; } diff --git a/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp b/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp index 8982b02f2b7..e4962c45921 100644 --- a/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp +++ b/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp @@ -53,6 +53,7 @@ struct ChangedBucketOwnershipHandlerTest : Test { void applyDistribution(Redundancy, NodeCount); void applyClusterState(const lib::ClusterState&); + void apply_cluster_state_bundle(std::shared_ptr<const lib::ClusterStateBundle>); document::BucketId nextOwnedBucket( uint16_t wantedOwner, @@ -84,6 +85,21 @@ struct ChangedBucketOwnershipHandlerTest : Test { return lib::ClusterState("distributor:4 storage:1 .0.s:d"); } + static std::shared_ptr<const lib::DistributionConfigBundle> make_distr_bundle(uint16_t node_count) { + return lib::DistributionConfigBundle::of(lib::Distribution::getDefaultDistributionConfig(1, node_count)); + } + + static std::shared_ptr<const lib::ClusterStateBundle> make_state_bundle_with_config( + std::string_view state_str, uint16_t node_count) + { + return std::make_shared<const lib::ClusterStateBundle>( + std::make_shared<const lib::ClusterState>(state_str), + lib::ClusterStateBundle::BucketSpaceStateMapping{}, + std::nullopt, + make_distr_bundle(node_count), + false); + } + void SetUp() override; }; @@ -194,6 +210,7 @@ hasOnlySetSystemStateCmdQueued(DummyStorageLink& link) { void ChangedBucketOwnershipHandlerTest::applyDistribution(Redundancy redundancy, NodeCount nodeCount) { + // TODO set distribution via state bundle instead _app->setDistribution(redundancy, nodeCount); _handler->storageDistributionChanged(); } @@ -205,6 +222,13 @@ ChangedBucketOwnershipHandlerTest::applyClusterState(const lib::ClusterState& st _handler->reloadClusterState(); } +void +ChangedBucketOwnershipHandlerTest::apply_cluster_state_bundle(std::shared_ptr<const lib::ClusterStateBundle> state_bundle) +{ + _app->set_cluster_state_bundle(std::move(state_bundle)); + _handler->reloadClusterState(); +} + TEST_F(ChangedBucketOwnershipHandlerTest, enumerate_buckets_belonging_on_changed_nodes) { lib::ClusterState stateBefore("distributor:4 storage:1"); applyDistribution(Redundancy(1), NodeCount(4)); @@ -225,7 +249,7 @@ TEST_F(ChangedBucketOwnershipHandlerTest, enumerate_buckets_belonging_on_changed EXPECT_TRUE(hasAbortedNoneOf(cmd, node2Buckets)); // Handler must swallow abort replies - _bottom->sendUp(api::StorageMessage::SP(cmd->makeReply().release())); + _bottom->sendUp(api::StorageMessage::SP(cmd->makeReply())); EXPECT_EQ(size_t(0), _top->getNumReplies()); } @@ -312,7 +336,7 @@ TEST_F(ChangedBucketOwnershipHandlerTest, ownership_changed_on_distributor_up_ed EXPECT_TRUE(hasAbortedNoneOf(cmd, node2Buckets)); // Handler must swallow abort replies - _bottom->sendUp(api::StorageMessage::SP(cmd->makeReply().release())); + _bottom->sendUp(api::StorageMessage::SP(cmd->makeReply())); EXPECT_EQ(0, _top->getNumReplies()); } @@ -345,6 +369,32 @@ TEST_F(ChangedBucketOwnershipHandlerTest, distribution_config_change_updates_own sendAndExpectAbortedCreateBucket(2); } +TEST_F(ChangedBucketOwnershipHandlerTest, distribution_config_via_state_bundle_change_updates_ownership) { + apply_cluster_state_bundle(make_state_bundle_with_config("version:2 distributor:3 storage:1", 3)); + // Apply new distribution config containing only 1 distributor, meaning + // any messages sent from >1 must be aborted. + // This test case is a bit dodgy since the CC should never send a state with more nodes in it than + // the distribution config allows for when _it_ is responsible for also sending the config. + apply_cluster_state_bundle(make_state_bundle_with_config("version:3 distributor:3 storage:1", 1)); + sendAndExpectAbortedCreateBucket(2); +} + +TEST_F(ChangedBucketOwnershipHandlerTest, ignore_internal_config_once_state_bundle_with_config_received) { + apply_cluster_state_bundle(make_state_bundle_with_config("version:2 distributor:3 storage:3", 1)); + applyDistribution(Redundancy(1), NodeCount(3)); + // Bundle config says 1 node, internal config says 3. Trust the bundle(tm). + sendAndExpectAbortedCreateBucket(2); +} + +TEST_F(ChangedBucketOwnershipHandlerTest, revert_to_internal_config_if_distribution_no_longer_received_in_state_bundle) { + apply_cluster_state_bundle(make_state_bundle_with_config("version:2 distributor:1 storage:3", 3)); + + applyDistribution(Redundancy(1), NodeCount(1)); // not yet used + applyClusterState(lib::ClusterState("version:3 distributor:3 storage:3")); // no bundle config; revert to internal + + sendAndExpectAbortedCreateBucket(2); +} + /** * Generate and dispatch a message of the given type with the provided * arguments as if that message was sent from distributor 1. Messages will diff --git a/storage/src/tests/storageserver/communicationmanagertest.cpp b/storage/src/tests/storageserver/communicationmanagertest.cpp index b741d79582f..7de9d7ca011 100644 --- a/storage/src/tests/storageserver/communicationmanagertest.cpp +++ b/storage/src/tests/storageserver/communicationmanagertest.cpp @@ -295,7 +295,7 @@ using vespa::config::content::core::BucketspacesConfigBuilder; namespace { -BucketspacesConfigBuilder::Documenttype doc_type(vespalib::stringref name, vespalib::stringref space) { +BucketspacesConfigBuilder::Documenttype doc_type(std::string_view name, std::string_view space) { BucketspacesConfigBuilder::Documenttype dt; dt.name = name; dt.bucketspace = space; diff --git a/storage/src/tests/storageserver/configurable_bucket_resolver_test.cpp b/storage/src/tests/storageserver/configurable_bucket_resolver_test.cpp index 499bc365edd..da8e76e7e93 100644 --- a/storage/src/tests/storageserver/configurable_bucket_resolver_test.cpp +++ b/storage/src/tests/storageserver/configurable_bucket_resolver_test.cpp @@ -76,7 +76,7 @@ using BucketSpacesConfigBuilder = vespa::config::content::core::BucketspacesConf namespace { -BucketSpacesConfigBuilder::Documenttype make_doc_type(vespalib::stringref name, vespalib::stringref space) { +BucketSpacesConfigBuilder::Documenttype make_doc_type(std::string_view name, std::string_view space) { BucketSpacesConfigBuilder::Documenttype doc_type; doc_type.name = name; doc_type.bucketspace = space; diff --git a/storage/src/tests/storageserver/rpc/caching_rpc_target_resolver_test.cpp b/storage/src/tests/storageserver/rpc/caching_rpc_target_resolver_test.cpp index 9b9b32a74ab..cfa56266848 100644 --- a/storage/src/tests/storageserver/rpc/caching_rpc_target_resolver_test.cpp +++ b/storage/src/tests/storageserver/rpc/caching_rpc_target_resolver_test.cpp @@ -17,7 +17,7 @@ public: Mappings mappings; uint32_t gen; MockMirror() : mappings(), gen(1) {} - SpecList lookup(vespalib::stringref pattern) const override { + SpecList lookup(std::string_view pattern) const override { auto itr = mappings.find(pattern); if (itr != mappings.end()) { return itr->second; diff --git a/storage/src/tests/storageserver/rpc/cluster_controller_rpc_api_service_test.cpp b/storage/src/tests/storageserver/rpc/cluster_controller_rpc_api_service_test.cpp index e59f6d22080..8aee487f526 100644 --- a/storage/src/tests/storageserver/rpc/cluster_controller_rpc_api_service_test.cpp +++ b/storage/src/tests/storageserver/rpc/cluster_controller_rpc_api_service_test.cpp @@ -125,7 +125,7 @@ struct SetStateFixture : FixtureBase { } }; -std::shared_ptr<const lib::ClusterState> state_of(vespalib::stringref state) { +std::shared_ptr<const lib::ClusterState> state_of(std::string_view state) { return std::make_shared<const lib::ClusterState>(state); } diff --git a/storage/src/tests/storageserver/rpc/storage_api_rpc_service_test.cpp b/storage/src/tests/storageserver/rpc/storage_api_rpc_service_test.cpp index 010f2b441ef..5852e27f78d 100644 --- a/storage/src/tests/storageserver/rpc/storage_api_rpc_service_test.cpp +++ b/storage/src/tests/storageserver/rpc/storage_api_rpc_service_test.cpp @@ -129,7 +129,7 @@ public: const SharedRpcResources& shared_rpc_resources() const noexcept { return *_shared_rpc_resources; } SharedRpcResources& shared_rpc_resources() noexcept { return *_shared_rpc_resources; } - void wait_until_visible_in_slobrok(vespalib::stringref id) { + void wait_until_visible_in_slobrok(std::string_view id) { const auto deadline = std::chrono::steady_clock::now() + slobrok_register_timeout; while (_shared_rpc_resources->slobrok_mirror().lookup(id).empty()) { if (std::chrono::steady_clock::now() > deadline) { diff --git a/storage/src/tests/storageserver/service_layer_error_listener_test.cpp b/storage/src/tests/storageserver/service_layer_error_listener_test.cpp index b84f96dd847..d7222700026 100644 --- a/storage/src/tests/storageserver/service_layer_error_listener_test.cpp +++ b/storage/src/tests/storageserver/service_layer_error_listener_test.cpp @@ -24,7 +24,7 @@ class TestShutdownListener public: TestShutdownListener() : _reason() {} - void requestShutdown(vespalib::stringref reason) override { + void requestShutdown(std::string_view reason) override { _reason = reason; } diff --git a/storage/src/tests/storageserver/statemanagertest.cpp b/storage/src/tests/storageserver/statemanagertest.cpp index 79246cb3ce1..c7f6ef0c5c0 100644 --- a/storage/src/tests/storageserver/statemanagertest.cpp +++ b/storage/src/tests/storageserver/statemanagertest.cpp @@ -10,6 +10,7 @@ #include <vespa/vdslib/state/clusterstate.h> #include <vespa/storage/storageserver/statemanager.h> #include <vespa/vespalib/data/slime/slime.h> +#include <vespa/config-stor-distribution.h> #include <vespa/vespalib/gtest/gtest.h> using storage::lib::NodeState; @@ -32,12 +33,29 @@ struct StateManagerTest : Test, NodeStateReporter { void SetUp() override; void TearDown() override; - static std::shared_ptr<api::SetSystemStateCommand> make_set_state_cmd(vespalib::stringref state_str, uint16_t cc_index) { + static std::shared_ptr<api::SetSystemStateCommand> make_set_state_cmd(std::string_view state_str, uint16_t cc_index) { auto cmd = std::make_shared<api::SetSystemStateCommand>(lib::ClusterState(state_str)); cmd->setSourceIndex(cc_index); return cmd; } + static std::shared_ptr<const lib::ClusterStateBundle> make_state_bundle_with_config( + std::string_view state_str, uint16_t num_nodes) + { + auto state = std::make_shared<const ClusterState>(state_str); + auto distr = lib::DistributionConfigBundle::of(lib::Distribution::getDefaultDistributionConfig(1, num_nodes)); + return std::make_shared<lib::ClusterStateBundle>(std::move(state), + lib::ClusterStateBundle::BucketSpaceStateMapping{}, + std::nullopt, std::move(distr), false); + } + + + static std::shared_ptr<api::SetSystemStateCommand> make_set_state_cmd_with_config( + std::string_view state_str, uint16_t num_nodes) + { + return std::make_shared<api::SetSystemStateCommand>(make_state_bundle_with_config(state_str, num_nodes)); + } + void get_single_reply(std::shared_ptr<api::StorageReply>& reply_out); void get_only_ok_reply(std::shared_ptr<api::StorageReply>& reply_out); void force_current_cluster_state_version(uint32_t version, uint16_t cc_index); @@ -56,6 +74,10 @@ struct StateManagerTest : Test, NodeStateReporter { void report(vespalib::JsonStream &) const override {} void extract_cluster_state_version_from_host_info(uint32_t& version_out); + + static vespalib::string to_string(const lib::Distribution::DistributionConfig& cfg) { + return lib::Distribution(cfg).serialized(); + } }; StateManagerTest::StateManagerTest() @@ -148,26 +170,107 @@ StateManagerTest::extract_cluster_state_version_from_host_info(uint32_t& version version_out = clusterStateVersionCursor.asLong(); } -TEST_F(StateManagerTest, cluster_state) { - std::shared_ptr<api::StorageReply> reply; - // Verify initial state on startup - auto currentState = _manager->getClusterStateBundle()->getBaselineClusterState(); +TEST_F(StateManagerTest, cluster_state_and_config_has_expected_values_at_bootstrap) { + auto initial_bundle = _manager->getClusterStateBundle(); + auto currentState = initial_bundle->getBaselineClusterState(); EXPECT_EQ("cluster:d", currentState->toString(false)); EXPECT_EQ(currentState->getVersion(), 0); + // Distribution config should be equal to the config the node is running with. + ASSERT_TRUE(initial_bundle->has_distribution_config()); + EXPECT_EQ(to_string(initial_bundle->distribution_config_bundle()->config()), + _node->getComponentRegister().getDistribution()->serialized()); + auto currentNodeState = _manager->getCurrentNodeState(); EXPECT_EQ("s:d", currentNodeState->toString(false)); +} + +TEST_F(StateManagerTest, can_receive_state_bundle_without_distribution_config) { + ClusterState send_state("version:2 distributor:1 storage:4 .2.s:m"); + auto cmd = std::make_shared<api::SetSystemStateCommand>(send_state); + _upper->sendDown(cmd); + std::shared_ptr<api::StorageReply> reply; + ASSERT_NO_FATAL_FAILURE(get_only_ok_reply(reply)); + + auto current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(send_state, *current_bundle->getBaselineClusterState()); + // Distribution config should be unchanged from bootstrap. + ASSERT_TRUE(current_bundle->has_distribution_config()); + EXPECT_EQ(to_string(current_bundle->distribution_config_bundle()->config()), + _node->getComponentRegister().getDistribution()->serialized()); + + auto current_node_state = _manager->getCurrentNodeState(); + EXPECT_EQ("s:m", current_node_state->toString(false)); +} + +TEST_F(StateManagerTest, can_receive_state_bundle_with_distribution_config) { + auto cmd = make_set_state_cmd_with_config("version:2 distributor:1 storage:4 .2.s:m", 5); + EXPECT_NE(to_string(cmd->getClusterStateBundle().distribution_config_bundle()->config()), + _node->getComponentRegister().getDistribution()->serialized()); + _upper->sendDown(cmd); + std::shared_ptr<api::StorageReply> reply; + ASSERT_NO_FATAL_FAILURE(get_only_ok_reply(reply)); + + auto current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(*current_bundle, cmd->getClusterStateBundle()); // also compares distribution configs +} - ClusterState sendState("storage:4 .2.s:m"); - auto cmd = std::make_shared<api::SetSystemStateCommand>(sendState); +TEST_F(StateManagerTest, receiving_cc_bundle_with_distribution_config_disables_node_distribution_config_propagation) { + auto cmd = make_set_state_cmd_with_config("version:2 distributor:1 storage:4 .2.s:m", 5); _upper->sendDown(cmd); + std::shared_ptr<api::StorageReply> reply; ASSERT_NO_FATAL_FAILURE(get_only_ok_reply(reply)); + // Explicitly setting distribution config should not propagate to the active state bundle + // since we've flipped to expecting config from the cluster controllers instead. + auto distr = std::make_shared<lib::Distribution>(lib::Distribution::getDefaultDistributionConfig(2, 7)); + _node->getComponentRegister().setDistribution(distr); + + auto current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(*current_bundle, cmd->getClusterStateBundle()); // unchanged +} + +TEST_F(StateManagerTest, internal_distribution_config_is_propagated_if_none_yet_received_from_cc) { + _upper->sendDown(make_set_state_cmd("version:10 distributor:1 storage:4", 0)); + std::shared_ptr<api::StorageReply> reply; + ASSERT_NO_FATAL_FAILURE(get_only_ok_reply(reply)); + + auto expected_bundle = make_state_bundle_with_config("version:10 distributor:1 storage:4", 7); + // Explicitly set internal config + _node->getComponentRegister().setDistribution(expected_bundle->distribution_config_bundle()->default_distribution_sp()); + _manager->storageDistributionChanged(); + + auto current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(*current_bundle, *expected_bundle); +} + +TEST_F(StateManagerTest, revert_to_internal_config_if_cc_no_longer_sends_distribution_config) { + // Initial state bundle _with_ distribution config + auto cmd = make_set_state_cmd_with_config("version:2 distributor:1 storage:4 .2.s:m", 5); + _upper->sendDown(cmd); + std::shared_ptr<api::StorageReply> reply; + ASSERT_NO_FATAL_FAILURE(get_only_ok_reply(reply)); + + auto current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(to_string(current_bundle->distribution_config_bundle()->config()), + to_string(cmd->getClusterStateBundle().distribution_config_bundle()->config())); + + // CC then sends a new bundle _without_ config + _upper->sendDown(make_set_state_cmd("version:3 distributor:1 storage:4", 0)); + ASSERT_NO_FATAL_FAILURE(get_only_ok_reply(reply)); + + // Config implicitly reverted to the active internal config + current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(to_string(current_bundle->distribution_config_bundle()->config()), + _node->getComponentRegister().getDistribution()->serialized()); - currentState = _manager->getClusterStateBundle()->getBaselineClusterState(); - EXPECT_EQ(sendState, *currentState); + // Explicitly set internal config + auto expected_bundle = make_state_bundle_with_config("version:3 distributor:1 storage:4", 7); + _node->getComponentRegister().setDistribution(expected_bundle->distribution_config_bundle()->default_distribution_sp()); + _manager->storageDistributionChanged(); - currentNodeState = _manager->getCurrentNodeState(); - EXPECT_EQ("s:m", currentNodeState->toString(false)); + // Internal config shall have taken effect, overriding that of the initial bundle + current_bundle = _manager->getClusterStateBundle(); + EXPECT_EQ(*current_bundle, *expected_bundle); } TEST_F(StateManagerTest, accept_lower_state_versions_if_strict_requirement_disabled) { diff --git a/storage/src/tests/visiting/commandqueuetest.cpp b/storage/src/tests/visiting/commandqueuetest.cpp index 74de7430c3f..a1dc97a859b 100644 --- a/storage/src/tests/visiting/commandqueuetest.cpp +++ b/storage/src/tests/visiting/commandqueuetest.cpp @@ -16,7 +16,7 @@ namespace storage { namespace { std::shared_ptr<api::CreateVisitorCommand> getCommand( - vespalib::stringref name, vespalib::duration timeout, + std::string_view name, vespalib::duration timeout, uint8_t priority = 0) { vespalib::asciistream ost; diff --git a/storage/src/tests/visiting/visitortest.cpp b/storage/src/tests/visiting/visitortest.cpp index 075ebd13741..3d34230c98f 100644 --- a/storage/src/tests/visiting/visitortest.cpp +++ b/storage/src/tests/visiting/visitortest.cpp @@ -80,7 +80,7 @@ struct VisitorTest : Test { VisitorOptions() {} - VisitorOptions& withVisitorType(vespalib::stringref type) { + VisitorOptions& withVisitorType(std::string_view type) { visitorType = type; return *this; } @@ -109,7 +109,7 @@ struct VisitorTest : Test { protected: void doTestVisitorInstanceHasConsistencyLevel( - vespalib::stringref visitorType, + std::string_view visitorType, spi::ReadConsistency expectedConsistency); template <typename T> @@ -850,7 +850,7 @@ TEST_F(VisitorTest, no_more_iterators_sent_while_memory_used_above_limit) { void VisitorTest::doTestVisitorInstanceHasConsistencyLevel( - vespalib::stringref visitorType, + std::string_view visitorType, spi::ReadConsistency expectedConsistency) { ASSERT_NO_FATAL_FAILURE(initializeTest()); |