Merge pull request #15466 from vespa-engine/geirst/simplify-storage-message-address

Simplify storage message address

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;
     }