Merge pull request #18603 from vespa-engine/arnej/implement-map-listener

let ServiceMapHistory implement MapListener API

4 files changed, 31 insertions, 24 deletions

diff --git a/slobrok/src/tests/service_map_history/service_map_history_test.cpp b/slobrok/src/tests/service_map_history/service_map_history_test.cpp
index e88f326a99b..cdd1b2f7311 100644
--- a/slobrok/src/tests/service_map_history/service_map_history_test.cpp
+++ b/slobrok/src/tests/service_map_history/service_map_history_test.cpp
@@ -89,12 +89,12 @@ TEST(ServiceMapHistoryTest, full_inspection) {
         for (int i = 0; i < 1984; ++i) {
             auto name = fmt("key/%d/name", i);
             auto spec = fmt("tcp/host%d.domain.tld:19099", 10000+i);
-            p.update(ServiceMapping{name, spec});
+            p.add(ServiceMapping{name, spec});
         }
         EXPECT_EQ(p.currentGen(), GenCnt(1985));
-        p.remove("key/666/name");
+        p.remove(ServiceMapping{"key/666/name", "tcp/host10666.domain.tld:19099"});
         EXPECT_EQ(p.currentGen(), GenCnt(1986));
-        p.update(ServiceMapping{"key/1969/name", "tcp/woodstock:19069"});
+        p.add(ServiceMapping{"key/1969/name", "tcp/woodstock:19069"});
         EXPECT_EQ(p.currentGen(), GenCnt(1987));
 
         auto map = dump(p);
@@ -187,7 +187,7 @@ TEST(ServiceMapHistoryTest, handlers_test) {
         EXPECT_FALSE(cantCancel);
 
         handler1.got_update = false;
-        p.update(ServiceMapping{"foo", "bar"});
+        p.add(ServiceMapping{"foo", "bar"});
         EXPECT_FALSE(handler1.got_update);
         EXPECT_TRUE(handler2.got_update);
         EXPECT_FALSE(handler3.got_update);
@@ -197,7 +197,7 @@ TEST(ServiceMapHistoryTest, handlers_test) {
         handler2.got_update = false;
         p.asyncGenerationDiff(&handler3, GenCnt(2));
         EXPECT_FALSE(handler3.got_update);
-        p.remove("foo");
+        p.remove(ServiceMapping{"foo", "bar"});
         EXPECT_FALSE(handler1.got_update);
         EXPECT_FALSE(handler2.got_update);
         EXPECT_TRUE(handler3.got_update);
diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp
index 5f26608c294..89675a543a7 100644
--- a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp
@@ -27,8 +27,9 @@ RpcServerMap::lookup(const std::string & name) const
 std::unique_ptr<NamedService>
 RpcServerMap::remove(const std::string & name)
 {
-    _visible_map.remove(name);
     auto service = std::move(_myrpcsrv_map[name]);
+    auto spec = service->getSpec();
+    _visible_map.remove(ServiceMapping{name, spec});
     _myrpcsrv_map.erase(name);
     return service;
 }
@@ -67,7 +68,7 @@ RpcServerMap::add(NamedService *rpcsrv)
     LOG_ASSERT(_myrpcsrv_map.find(name) == _myrpcsrv_map.end());
 
     removeReservation(name);
-    _visible_map.update(ServiceMapping{name, rpcsrv->getSpec()});
+    _visible_map.add(ServiceMapping{name, rpcsrv->getSpec()});
 }
 
 void
@@ -80,9 +81,8 @@ RpcServerMap::addNew(std::unique_ptr<ManagedRpcServer> rpcsrv)
 
     if (oldman) {
         const ReservedName *oldres = _reservations[name].get();
-        _visible_map.remove(name);
-
         const std::string &spec = rpcsrv->getSpec();
+        _visible_map.remove(ServiceMapping{name, spec});
         const std::string &oldname = oldman->getName();
         const std::string &oldspec = oldman->getSpec();
         if (spec != oldspec)  {
diff --git a/slobrok/src/vespa/slobrok/server/service_map_history.cpp b/slobrok/src/vespa/slobrok/server/service_map_history.cpp
index 3882a41bde5..5a2f8c6ff43 100644
--- a/slobrok/src/vespa/slobrok/server/service_map_history.cpp
+++ b/slobrok/src/vespa/slobrok/server/service_map_history.cpp
@@ -82,24 +82,29 @@ bool ServiceMapHistory::cancel(DiffCompletionHandler *handler) {
     return (removed > 0);
 }
 
-void ServiceMapHistory::remove(const vespalib::string &name) {
+void ServiceMapHistory::remove(const ServiceMapping &mapping) {
     {
         std::lock_guard guard(_lock);
-        auto iter = _map.find(name);
+        auto iter = _map.find(mapping.name);
         if (iter == _map.end()) {
-            LOG(warning, "already removed: %s", name.c_str());
-            // already removed
-            return;
+            LOG(debug, "already removed: %s", mapping.name.c_str());
+            return; // already removed
         }
+        LOG_ASSERT(iter->second == mapping.spec);
         _map.erase(iter);
-        _log.add(name);
+        _log.add(mapping.name);
     }
     notify_updated();
 }
 
-void ServiceMapHistory::update(const ServiceMapping &mapping) {
+void ServiceMapHistory::add(const ServiceMapping &mapping) {
     {
         std::lock_guard guard(_lock);
+        auto iter = _map.find(mapping.name);
+        if (iter != _map.end() && iter->second == mapping.spec) {
+            // already ok
+            return;
+        }
         _map.insert_or_assign(mapping.name, mapping.spec);
         _log.add(mapping.name);
     }
diff --git a/slobrok/src/vespa/slobrok/server/service_map_history.h b/slobrok/src/vespa/slobrok/server/service_map_history.h
index e1f20e0d8a4..9026586b945 100644
--- a/slobrok/src/vespa/slobrok/server/service_map_history.h
+++ b/slobrok/src/vespa/slobrok/server/service_map_history.h
@@ -7,6 +7,7 @@
 #include <vespa/vespalib/util/arrayqueue.hpp>
 #include <map>
 #include <mutex>
+#include "map_listener.h"
 #include "service_mapping.h"
 #include "map_diff.h"
 
@@ -16,8 +17,7 @@ namespace slobrok {
  * @class ServiceMapHistory
  * @brief API to generate incremental updates for a collection of name->spec mappings
  **/
-
-class ServiceMapHistory
+class ServiceMapHistory : public MapListener
 {
 public:
     using Generation = vespalib::GenCnt;
@@ -66,6 +66,11 @@ public:
     ~ServiceMapHistory();
 
     /**
+     * Get diff from generation fromGen (sync version).
+     **/
+    MapDiff makeDiffFrom(const Generation &fromGen) const;
+
+    /**
      * Ask for notification when the history has changes newer than fromGen.
      * Note that if there are any changes in the history already, the callback
      * will happen immediately (inside asyncGenerationDiff).
@@ -78,17 +83,14 @@ public:
      **/
     bool cancel(DiffCompletionHandler *handler);
 
-    /** add or update name->spec mapping */
-    void update(const ServiceMapping &mapping);
+    /** add name->spec mapping */
+    void add(const ServiceMapping &mapping) override;
 
     /** remove mapping for name */
-    void remove(const vespalib::string &name);
+    void remove(const ServiceMapping &mapping) override;
 
     /** For unit testing only: */
     Generation currentGen() const { return myGen(); }
-
-private:
-    MapDiff makeDiffFrom(const Generation &fromGen) const;
 };
 
 //-----------------------------------------------------------------------------