update after review

* MappingMonitorListener -> MappingMonitorOwner
* construct with factory
* RpcMappingMonitor always has an owner
* refactor common lookup into active() method

3 files changed, 28 insertions, 40 deletions

diff --git a/slobrok/src/vespa/slobrok/server/mapping_monitor.h b/slobrok/src/vespa/slobrok/server/mapping_monitor.h
index d7e1a6975cb..035023f32fa 100644
--- a/slobrok/src/vespa/slobrok/server/mapping_monitor.h
+++ b/slobrok/src/vespa/slobrok/server/mapping_monitor.h
@@ -3,23 +3,26 @@
 #pragma once
 
 #include "service_mapping.h"
+#include <memory>
+#include <functional>
 
 namespace slobrok {
 
-struct MappingMonitorListener {
+struct MappingMonitorOwner {
     virtual void up(const ServiceMapping& mapping) = 0;
     virtual void down(const ServiceMapping& mapping) = 0;
 protected:
-    ~MappingMonitorListener() = default;
+    ~MappingMonitorOwner() = default;
 };
 
 struct MappingMonitor {
-    virtual void target(MappingMonitorListener *listener) = 0;
+    using UP = std::unique_ptr<MappingMonitor>;
     virtual void start(const ServiceMapping& mapping) = 0;
     virtual void stop(const ServiceMapping& mapping) = 0;
-protected:
-    ~MappingMonitor() = default;
+    virtual ~MappingMonitor() = default;
 };
 
+using MappingMonitorFactory = std::function<MappingMonitor::UP(MappingMonitorOwner &)>;
+
 } // namespace slobrok
 
diff --git a/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.cpp b/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.cpp
index ec56db709f5..80c1a7d445a 100644
--- a/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.cpp
+++ b/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.cpp
@@ -12,25 +12,15 @@ void RpcMappingMonitor::DelayedTasks::PerformTask() {
     std::swap(deleteAfterSwap, _deleteList);
 }
 
-RpcMappingMonitor::RpcMappingMonitor(FRT_Supervisor &orb)
+RpcMappingMonitor::RpcMappingMonitor(FRT_Supervisor &orb, MappingMonitorOwner &owner)
   : _orb(orb),
     _delayedTasks(orb.GetScheduler()),
     _map(),
-    _target(nullptr)
+    _owner(owner)
 {}
 
 RpcMappingMonitor::~RpcMappingMonitor() = default;
 
-void RpcMappingMonitor::target(MappingMonitorListener *listener) {
-    if (listener == nullptr) {
-        LOG_ASSERT(_target != nullptr);
-    } else {
-        LOG_ASSERT(_target == nullptr);
-    }
-    _target = listener;
-    LOG(debug, "new target %p", _target);
-}
-
 void RpcMappingMonitor::start(const ServiceMapping& mapping) {
     LOG(spam, "start %s->%s", mapping.name.c_str(), mapping.spec.c_str());
     LOG_ASSERT(_map.find(mapping) == _map.end());
@@ -46,39 +36,32 @@ void RpcMappingMonitor::stop(const ServiceMapping& mapping) {
     _map.erase(iter);
 }
 
+
+bool RpcMappingMonitor::active(const ServiceMapping &mapping, ManagedRpcServer *rpcsrv) const {
+    auto iter = _map.find(mapping);
+    if (iter == _map.end()) {
+        return false;
+    }
+    return iter->second.get() == rpcsrv;
+}
+        
 void RpcMappingMonitor::notifyFailedRpcSrv(ManagedRpcServer *rpcsrv, std::string errmsg) {
     ServiceMapping mapping{rpcsrv->getName(), rpcsrv->getSpec()};
     LOG(spam, "notifyFailed %s->%s", mapping.name.c_str(), mapping.spec.c_str());
-    Map::const_iterator iter = _map.find(mapping);
-    if (iter == _map.end()) {
-        return;
-    }
-    const MUP & managedRpcServer = iter->second;
-    if (managedRpcServer.get() != rpcsrv) {
-        return;
-    }
-    if (_target) {
+    if (active(mapping, rpcsrv)) {
         LOG(debug, "service %s [at %s] failed: %s",
             mapping.name.c_str(), mapping.spec.c_str(), errmsg.c_str());
-        _target->down(mapping);
+        _owner.down(mapping);
     }
 }
 
 void RpcMappingMonitor::notifyOkRpcSrv(ManagedRpcServer *rpcsrv) {
     ServiceMapping mapping{rpcsrv->getName(), rpcsrv->getSpec()};
     LOG(spam, "notifyOk %s->%s", mapping.name.c_str(), mapping.spec.c_str());
-    Map::const_iterator iter = _map.find(mapping);
-    if (iter == _map.end()) {
-        return;
-    }
-    const MUP & managedRpcServer = iter->second;
-    if (managedRpcServer.get() != rpcsrv) {
-        return;
-    }
-    if (_target) {
+    if (active(mapping, rpcsrv)) {
         LOG(debug, "service %s [at %s] up ok -> target",
             mapping.name.c_str(), mapping.spec.c_str());
-        _target->up(mapping);
+        _owner.up(mapping);
     }
 }
 
diff --git a/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.h b/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.h
index 36043ac41b2..2f05a459729 100644
--- a/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.h
+++ b/slobrok/src/vespa/slobrok/server/rpc_mapping_monitor.h
@@ -41,12 +41,14 @@ private:
     FRT_Supervisor& _orb;
     DelayedTasks _delayedTasks;
     Map _map;
-    MappingMonitorListener* _target;
+    MappingMonitorOwner &_owner;
+
+    bool active(const ServiceMapping &mapping, ManagedRpcServer *rpcsrv) const;
+
 public:
-    RpcMappingMonitor(FRT_Supervisor &orb);
+    RpcMappingMonitor(FRT_Supervisor &orb, MappingMonitorOwner &owner);
     ~RpcMappingMonitor();
 
-    void target(MappingMonitorListener *listener) override;
     void start(const ServiceMapping& mapping) override;
     void stop(const ServiceMapping& mapping) override;