From 840967e66330b273fb8f8a73565d60d13a4f786d Mon Sep 17 00:00:00 2001 From: Arne Juul Date: Wed, 14 Jul 2021 09:36:20 +0000 Subject: let ServiceMapHistory implement MapListener API --- .../service_map_history/service_map_history_test.cpp | 10 +++++----- slobrok/src/vespa/slobrok/server/rpc_server_map.cpp | 8 ++++---- .../src/vespa/slobrok/server/service_map_history.cpp | 19 ++++++++++++------- .../src/vespa/slobrok/server/service_map_history.h | 18 ++++++++++-------- 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 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 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 #include #include +#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; @@ -65,6 +65,11 @@ public: ServiceMapHistory(); ~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 @@ -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; }; //----------------------------------------------------------------------------- -- cgit v1.2.3