diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2020-11-25 16:52:57 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-11-25 16:52:57 +0100 |
commit | 3ca30562411372bb23d3d871a24111e20f79892b (patch) | |
tree | dff3e2d37a539dabc9477247554e7387bbb7b401 /storageapi/src | |
parent | 9b6a40a34d92bb723587b55fad9b4954dc5f275d (diff) | |
parent | e9e1b5ef1e6cd77ff5e198ac2ff70449499371af (diff) |
Merge pull request #15466 from vespa-engine/geirst/simplify-storage-message-address
Simplify storage message address
Diffstat (limited to 'storageapi/src')
4 files changed, 28 insertions, 33 deletions
diff --git a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp index 68eb4a90c9f..e413a62ae39 100644 --- a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp +++ b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp @@ -117,7 +117,7 @@ namespace { TEST_F(StorageProtocolTest, testAddress50) { StorageMessageAddress address("foo", lib::NodeType::STORAGE, 3); EXPECT_EQ(vespalib::string("storage/cluster.foo/storage/3/default"), - address.getRoute().toString()); + address.to_mbus_route().toString()); } template<typename Command> std::shared_ptr<Command> diff --git a/storageapi/src/tests/messageapi/storage_message_address_test.cpp b/storageapi/src/tests/messageapi/storage_message_address_test.cpp index 55adcf46c0d..f7f254e9119 100644 --- a/storageapi/src/tests/messageapi/storage_message_address_test.cpp +++ b/storageapi/src/tests/messageapi/storage_message_address_test.cpp @@ -22,17 +22,17 @@ TEST(StorageMessageAddressTest, storage_hash_covers_all_expected_fields) { hash_of("foo", lib::NodeType::DISTRIBUTOR, 0)); EXPECT_EQ(hash_of("foo", lib::NodeType::STORAGE, 123), hash_of("foo", lib::NodeType::STORAGE, 123)); + EXPECT_EQ(hash_of("foo", lib::NodeType::STORAGE, 0), + hash_of("bar", lib::NodeType::STORAGE, 0)); // These tests are all true with extremely high probability, though they do // depend on a hash function that may inherently cause collisions. EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0), - hash_of("bar", lib::NodeType::STORAGE, 0)); - EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0), hash_of("foo", lib::NodeType::DISTRIBUTOR, 0)); EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0), hash_of("foo", lib::NodeType::STORAGE, 1)); - EXPECT_EQ(112u, sizeof(StorageMessageAddress)); + EXPECT_EQ(88u, sizeof(StorageMessageAddress)); EXPECT_EQ(64u, sizeof(StorageMessage)); EXPECT_EQ(16u, sizeof(mbus::Trace)); } diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp index 16f35b8d939..9c5df379d22 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp +++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp @@ -139,15 +139,6 @@ MessageType::print(std::ostream& out, bool verbose, const std::string& indent) c out << ")"; } -StorageMessageAddress::StorageMessageAddress(const mbus::Route& route) - : _route(route), - _cluster(), - _precomputed_storage_hash(0), - _type(nullptr), - _protocol(DOCUMENT), - _index(0xFFFF) -{ } - std::ostream & operator << (std::ostream & os, const StorageMessageAddress & addr) { return os << addr.toString(); } @@ -160,39 +151,45 @@ createAddress(vespalib::stringref cluster, const lib::NodeType& type, uint16_t i return os.str(); } +size_t +calculate_node_hash(const lib::NodeType& type, uint16_t index) +{ + uint16_t buf[] = { type, index }; + return vespalib::hashValue(&buf, sizeof(buf)); +} + // TODO we ideally want this removed. Currently just in place to support usage as map key when emplacement not available StorageMessageAddress::StorageMessageAddress() - : _route(), - _cluster(), + : _cluster(), _precomputed_storage_hash(0), _type(nullptr), _protocol(Protocol::STORAGE), _index(0) {} - StorageMessageAddress::StorageMessageAddress(vespalib::stringref cluster, const lib::NodeType& type, uint16_t index, Protocol protocol) - : _route(), - _cluster(cluster), - _precomputed_storage_hash(0), + : _cluster(cluster), + _precomputed_storage_hash(calculate_node_hash(type, index)), _type(&type), _protocol(protocol), _index(index) { - std::vector<mbus::IHopDirective::SP> directives; - auto address_as_str = createAddress(cluster, type, index); - // We reuse the string representation and pass it to vespalib's hashValue instead of - // explicitly combining a running hash over the individual fields. This is because - // hashValue internally uses xxhash, which offers great dispersion of bits even for - // minimal changes in the input (such as single bit differences in the index). - _precomputed_storage_hash = vespalib::hashValue(address_as_str.data(), address_as_str.size()); - directives.emplace_back(std::make_shared<mbus::VerbatimDirective>(std::move(address_as_str))); - _route.addHop(mbus::Hop(std::move(directives), false)); } StorageMessageAddress::~StorageMessageAddress() = default; +mbus::Route +StorageMessageAddress::to_mbus_route() const +{ + mbus::Route result; + auto address_as_str = createAddress(_cluster, *_type, _index); + std::vector<mbus::IHopDirective::SP> directives; + directives.emplace_back(std::make_shared<mbus::VerbatimDirective>(std::move(address_as_str))); + result.addHop(mbus::Hop(std::move(directives), false)); + return result; +} + uint16_t StorageMessageAddress::getIndex() const { @@ -251,7 +248,7 @@ StorageMessageAddress::print(vespalib::asciistream & out) const out << "Document protocol"; } if (!_type) { - out << ", " << _route.toString() << ")"; + out << ", " << to_mbus_route().toString() << ")"; } else { out << ", cluster " << _cluster << ", nodetype " << *_type << ", index " << _index << ")"; diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h index a2227246a52..98552e473c1 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h +++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h @@ -266,7 +266,6 @@ public: enum Protocol { STORAGE, DOCUMENT }; private: - mbus::Route _route; vespalib::string _cluster; // Used for internal VDS addresses only size_t _precomputed_storage_hash; @@ -276,7 +275,6 @@ private: public: StorageMessageAddress(); // Only to be used when transient default ctor semantics are needed by containers - StorageMessageAddress(const mbus::Route& route); StorageMessageAddress(vespalib::stringref clusterName, const lib::NodeType& type, uint16_t index, Protocol protocol = STORAGE); @@ -284,13 +282,13 @@ public: void setProtocol(Protocol p) { _protocol = p; } - const mbus::Route& getRoute() const { return _route; } + mbus::Route to_mbus_route() const; Protocol getProtocol() const { return _protocol; } uint16_t getIndex() const; const lib::NodeType& getNodeType() const; const vespalib::string& getCluster() const; - // Returns precomputed hash over <cluster, type, index> tuple. Other fields not included. + // Returns precomputed hash over <type, index> pair. Other fields not included. [[nodiscard]] size_t internal_storage_hash() const noexcept { return _precomputed_storage_hash; } |