Fixed to remove need for calling updateViews after map registration

5 files changed, 150 insertions, 88 deletions

diff --git a/cpp/src/Ice/InstrumentationI.cpp b/cpp/src/Ice/InstrumentationI.cpp
index a5121ff7e23..3b7ae11a495 100644
--- a/cpp/src/Ice/InstrumentationI.cpp
+++ b/cpp/src/Ice/InstrumentationI.cpp
@@ -750,7 +750,6 @@ CommunicatorObserverI::CommunicatorObserverI(const MetricsAdminIPtr& metrics) :
     _endpointLookups(metrics, "EndpointLookup")
 {
     _invocations.registerSubMap<Metrics>("Remote", &InvocationMetrics::remotes);
-    _metrics->updateViews();
 }
 
 void
diff --git a/cpp/src/Ice/MetricsAdminI.cpp b/cpp/src/Ice/MetricsAdminI.cpp
index 493f0a30e40..53448eb1173 100644
--- a/cpp/src/Ice/MetricsAdminI.cpp
+++ b/cpp/src/Ice/MetricsAdminI.cpp
@@ -236,11 +236,9 @@ MetricsViewI::MetricsViewI(const string& name) : _name(name)
 {
 }
 
-void
-MetricsViewI::update(const PropertiesPtr& properties,  
-                     const map<string, MetricsMapFactoryPtr>& factories,
-                     set<MetricsMapFactoryPtr>& updatedMaps,
-                     const Ice::LoggerPtr& logger)
+bool
+MetricsViewI::addOrUpdateMap(const PropertiesPtr& properties, const string& mapName, 
+                             const MetricsMapFactoryPtr& factory, const Ice::LoggerPtr& logger)
 {
     //
     // Add maps to views configured with the given map.
@@ -248,53 +246,61 @@ MetricsViewI::update(const PropertiesPtr& properties,
     const string viewPrefix = "IceMX.Metrics." + _name + ".";
     const string mapsPrefix = viewPrefix + "Map.";
     PropertyDict mapsProps = properties->getPropertiesForPrefix(mapsPrefix);
-    for(map<string, MetricsMapFactoryPtr>::const_iterator p = factories.begin(); p != factories.end(); ++p)
+
+    string mapPrefix;
+    PropertyDict mapProps;
+    if(!mapsProps.empty())
     {
-        const string& mapName = p->first;
-        string mapPrefix;
-        PropertyDict mapProps;
-        if(!mapsProps.empty())
+        mapPrefix = mapsPrefix + mapName + ".";
+        mapProps = properties->getPropertiesForPrefix(mapPrefix);
+        if(mapProps.empty())
         {
-            mapPrefix = mapsPrefix + mapName + ".";
-            mapProps = properties->getPropertiesForPrefix(mapPrefix);
-            if(mapProps.empty())
-            {
-                // This map isn't configured anymore for this view.
-                updatedMaps.insert(p->second);
-                _maps.erase(mapName);
-                continue; 
-            }
-        }
-        else
-        {
-            mapPrefix = viewPrefix;
-            mapProps = properties->getPropertiesForPrefix(mapPrefix);
+            // This map isn't configured for this view.
+            _maps.erase(mapName);
+            return true;
         }
+    }
+    else
+    {
+        mapPrefix = viewPrefix;
+        mapProps = properties->getPropertiesForPrefix(mapPrefix);
+    }
 
-        map<string, MetricsMapIPtr>::iterator q = _maps.find(mapName);
-        if(q != _maps.end() && q->second->getProperties() == mapProps)
-        {
-            continue; // The map configuration didn't change, no need to re-create.
-        }
+    map<string, MetricsMapIPtr>::iterator q = _maps.find(mapName);
+    if(q != _maps.end() && q->second->getProperties() == mapProps)
+    {
+        return false; // The map configuration didn't change, no need to re-create.
+    }
 
-        try
-        {
-            _maps[mapName] = p->second->create(mapPrefix, properties);
-        }
-        catch(const std::exception& ex)
-        {
-            Ice::Warning warn(logger);
-            warn << "unexpected exception while creating metrics map:\n" << ex;
-            _maps.erase(mapName);
-        }
-        catch(const string& msg)
-        {
-            Ice::Warning warn(logger);
-            warn << msg;
-            _maps.erase(mapName);
-        }
-        updatedMaps.insert(p->second);
+    try
+    {
+        _maps[mapName] = factory->create(mapPrefix, properties);
+    }
+    catch(const std::exception& ex)
+    {
+        Ice::Warning warn(logger);
+        warn << "unexpected exception while creating metrics map:\n" << ex;
+        _maps.erase(mapName);
+    }
+    catch(const string& msg)
+    {
+        Ice::Warning warn(logger);
+        warn << msg;
+        _maps.erase(mapName);
     }
+    return true;
+}
+
+bool
+MetricsViewI::removeMap(const string& mapName)
+{
+    map<string, MetricsMapIPtr>::iterator q = _maps.find(mapName);
+    if(q != _maps.end())
+    {
+        _maps.erase(q);
+        return true;
+    }
+    return false;
 }
 
 MetricsView
@@ -404,7 +410,14 @@ MetricsAdminI::updateViews()
             {
                 q = views.insert(make_pair(viewName, q->second)).first;
             }
-            q->second->update(_properties, _factories, updatedMaps, _logger);
+
+            for(map<string, MetricsMapFactoryPtr>::const_iterator p = _factories.begin(); p != _factories.end(); ++p)
+            {
+                if(q->second->addOrUpdateMap(_properties, p->first, p->second, _logger))
+                {
+                    updatedMaps.insert(p->second);
+                }
+            }
         }
         _views.swap(views);
         
@@ -433,6 +446,28 @@ MetricsAdminI::updateViews()
     }
 }
 
+void
+MetricsAdminI::unregisterMap(const std::string& mapName)
+{
+    bool updated;
+    MetricsMapFactoryPtr factory;
+    {
+        Lock sync(*this);
+        map<string, MetricsMapFactoryPtr>::iterator p = _factories.find(mapName);
+        if(p == _factories.end())
+        {
+            return;
+        }
+        factory = p->second;
+        _factories.erase(p);
+        updated = removeMap(mapName);
+    }
+    if(updated)
+    {
+        factory->update();
+    }
+}
+
 StringSeq
 MetricsAdminI::getMetricsViewNames(const Current&)
 {
@@ -532,3 +567,24 @@ MetricsAdminI::updated(const PropertyDict& props)
     }
 }
 
+bool
+MetricsAdminI::addOrUpdateMap(const std::string& mapName, const MetricsMapFactoryPtr& factory)
+{
+    bool updated = false;
+    for(std::map<string, MetricsViewIPtr>::const_iterator p = _views.begin(); p != _views.end(); ++p)
+    {
+        updated |= p->second->addOrUpdateMap(_properties, mapName, factory, _logger);
+    }
+    return updated;
+}
+
+bool
+MetricsAdminI::removeMap(const std::string& mapName)
+{
+    bool updated = false;
+    for(std::map<string, MetricsViewIPtr>::const_iterator p = _views.begin(); p != _views.end(); ++p)
+    {
+        updated |= p->second->removeMap(mapName);
+    }
+    return updated;
+}
diff --git a/cpp/src/Ice/MetricsAdminI.h b/cpp/src/Ice/MetricsAdminI.h
index 260d283a2e2..0ed8a5a050d 100644
--- a/cpp/src/Ice/MetricsAdminI.h
+++ b/cpp/src/Ice/MetricsAdminI.h
@@ -495,8 +495,9 @@ public:
     
     MetricsViewI(const std::string&);
 
-    void update(const Ice::PropertiesPtr&, const std::map<std::string, MetricsMapFactoryPtr>&, 
-                std::set<MetricsMapFactoryPtr>&, const Ice::LoggerPtr&);
+    bool addOrUpdateMap(const Ice::PropertiesPtr&, const std::string&, const MetricsMapFactoryPtr&, 
+                        const Ice::LoggerPtr&);
+    bool removeMap(const std::string&);
 
     MetricsView getMetrics();
     MetricsFailuresSeq getFailures(const std::string&);
@@ -524,8 +525,18 @@ public:
     template<class MetricsType> void 
     registerMap(const std::string& map, Updater* updater)
     {
-        Lock sync(*this);
-        _factories[map] = new MetricsMapFactoryT<MetricsType>(updater);
+        bool update;
+        MetricsMapFactoryPtr factory;
+        {
+            Lock sync(*this);
+            factory = new MetricsMapFactoryT<MetricsType>(updater);
+            _factories[map] = factory;
+            addOrUpdateMap(map, factory);
+        }
+        if(update)
+        {
+            factory->update();
+        }
     }
 
     template<class MemberMetricsType, class MetricsType> void
@@ -534,18 +545,14 @@ public:
         Lock sync(*this);
 
         std::map<std::string, MetricsMapFactoryPtr>::const_iterator p = _factories.find(map);
-        assert(p != _factories.end());
-
-        MetricsMapFactoryT<MetricsType>* factory = dynamic_cast<MetricsMapFactoryT<MetricsType>*>(p->second.get());
-        factory->template registerSubMap<MemberMetricsType>(subMap, member);
+        if(p != _factories.end())
+        {
+            MetricsMapFactoryT<MetricsType>* factory = dynamic_cast<MetricsMapFactoryT<MetricsType>*>(p->second.get());
+            factory->template registerSubMap<MemberMetricsType>(subMap, member);
+        }
     }
 
-    void
-    unregisterMap(const std::string& map)
-    {
-        Lock sync(*this);
-        _factories.erase(map);
-    }
+    void unregisterMap(const std::string&);
 
     virtual Ice::StringSeq getMetricsViewNames(const ::Ice::Current&);
     virtual MetricsView getMetricsView(const std::string&, const ::Ice::Current&);
@@ -563,6 +570,9 @@ private:
 
     void updated(const Ice::PropertyDict&);
 
+    bool addOrUpdateMap(const std::string&, const MetricsMapFactoryPtr&);
+    bool removeMap(const std::string&);
+
     std::map<std::string, MetricsViewIPtr> _views;
     std::map<std::string, MetricsMapFactoryPtr> _factories;
 
diff --git a/cpp/src/Ice/MetricsObserverI.h b/cpp/src/Ice/MetricsObserverI.h
index 653b0535d93..07b19567652 100644
--- a/cpp/src/Ice/MetricsObserverI.h
+++ b/cpp/src/Ice/MetricsObserverI.h
@@ -31,14 +31,6 @@ public:
     virtual std::string operator()(const std::string&) const = 0;
 };
 
-class Updater : public IceUtil::Shared
-{
-public:
-
-    virtual void update() = 0;
-};
-typedef IceUtil::Handle<Updater> UpdaterPtr;
-
 template<typename T> class MetricsHelperT : public MetricsHelper
 {
 public:
@@ -268,6 +260,14 @@ protected:
     };
 };
 
+class Updater : public IceUtil::Shared
+{
+public:
+
+    virtual void update() = 0;
+};
+typedef IceUtil::Handle<Updater> UpdaterPtr;
+
 template<typename T> class UpdaterT : public Updater
 {
 public:
@@ -288,7 +288,17 @@ private:
     void (T::*_fn)();
 };
 
-class ObserverI;
+template<typename T> Updater*
+newUpdater(const IceUtil::Handle<T>& updater, void (T::*fn)())
+{
+    return new UpdaterT<T>(updater.get(), fn);
+}
+
+template<typename T> Updater*
+newUpdater(const IceInternal::Handle<T>& updater, void (T::*fn)())
+{
+    return new UpdaterT<T>(updater.get(), fn);
+}
 
 template<typename T> class ObserverT : virtual public Ice::Instrumentation::Observer
 {
@@ -413,22 +423,6 @@ private:
     IceUtilInternal::StopWatch _watch;
 };
 
-class ObserverI : virtual public Ice::Instrumentation::Observer, public ObserverT<Metrics>
-{
-};
-
-template<typename T> Updater*
-newUpdater(const IceUtil::Handle<T>& updater, void (T::*fn)())
-{
-    return new UpdaterT<T>(updater.get(), fn);
-}
-
-template<typename T> Updater*
-newUpdater(const IceInternal::Handle<T>& updater, void (T::*fn)())
-{
-    return new UpdaterT<T>(updater.get(), fn);
-}
-
 template<typename ObserverImplType> 
 class ObserverFactoryT : public Updater, private IceUtil::Mutex
 {
@@ -566,6 +560,10 @@ private:
     UpdaterPtr _updater;
 };
 
+class ObserverI : virtual public Ice::Instrumentation::Observer, public ObserverT<Metrics>
+{
+};
+
 }
 
 #endif
diff --git a/cpp/src/IceStorm/InstrumentationI.cpp b/cpp/src/IceStorm/InstrumentationI.cpp
index b7bbae60410..e310557dbce 100644
--- a/cpp/src/IceStorm/InstrumentationI.cpp
+++ b/cpp/src/IceStorm/InstrumentationI.cpp
@@ -317,7 +317,6 @@ TopicManagerObserverI::TopicManagerObserverI(const MetricsAdminIPtr& metrics) :
     _topics(metrics, "Topic"),
     _subscribers(metrics, "Subscriber")
 {
-    _metrics->updateViews();
 }
 
 void