Merge lp:~radix/landscape-client/fix-landscape-client-startup into lp:~landscape/landscape-client/trunk

[Christopher Armstrong]

Get rid of implicit population of the components registry; it's now explicitly created in a function, and used directly when it's needed.

Unfortunately no tests with this one, but it's an almost-pure code refactor.

Basically, the module that happened to populate the global registry of components wasn't being imported any more. I decided to get rid of the global registry entirely as a solution :) now there's just a function that returns the registry (as a dict) when called. now there's no more dependency on import ordering.

[Free Ekanayaka (free.ekanayaka)]

Looks good! +1

[1]

+
+def get_component_registry():
+ return {
+ "broker": RemoteBrokerConnector,
+ "client": RemoteClientConnector,
+ "monitor": RemoteMonitorConnector,
+ "manager": RemoteManagerConnector
+ }

You could use the "name" class attribute of the "component" class attribute, to avoid duplication, e.g.:

return {
    RemoteBrokerConnector.component.name: RemoteBrokerConnector,
    // etc.
}

Later on I believe we could get rid of the ComponentConnector sub-classes too relatively easy (as you did for the ComponentPublisher-based ones), but it'd probably require dropping the RemoteBroker class as pre-requisite. Its two methods could be moved to the Monitor/Manager classes where they are used (just thinking loud).

[2]

+ from landscape.broker.amp import get_component_registry
+ self.connectors_registry = get_component_registry()

I believe one reason for which we had the ComponentRegistry was to avoid circular imports like this. So it'd be great to have a way of avoiding dependency on import ordering while still not introducing circular imports (maybe dropping the ComponentConnector sub-classes will do it, not sure).

review: Approve

[Alberto Donato (ack)]

+1, looks good!

#1:
+def get_component_registry():
+ return {
+ "broker": RemoteBrokerConnector,
+ "client": RemoteClientConnector,
+ "monitor": RemoteMonitorConnector,
+ "manager": RemoteManagerConnector
+ }

You could write this as

+COMPONENTS = [
+ RemoteBrokerConnector, RemoteClientConnector, RemoteMonitorConnector, RemoteManagerConnector]
+
+def get_component_registry():
+ return dict(
+ (connector_class.component.name, connector_class)
+ for connector_class in COMPONENTS)

#2:
landscape/broker/server.py:305:1: W391 blank line at end of file

review: Approve

[lp:~radix/landscape-client/fix-landscape-client-startup]

677. By Christopher Armstrong on 2013-05-16

Address comments:
 - don't duplicate name information in get_component_registry
 - lint

[Christopher Armstrong (radix)]

free [1] fixed

free [2]

I actually almost removed ComponentConnector subclasses, after my branch to remove the ComponentPublisher subclasses. It was a bit trickier than I wanted to get into at the time, mostly because it involved updating watchdog and configuration code, as well as this registry code. Maybe I'll take another look at it before I leave.

ack [1] fixed
ack [2] fixed

=== modified file 'landscape/amp.py'
--- landscape/amp.py	2013-05-14 22:57:15 +0000
+++ landscape/amp.py	2013-05-16 16:41:25 +0000
@@ -151,31 +151,5 @@
             self._connector = None
 
 
-class ComponentsRegistry(object):
-    """
-    A global registry for looking up Landscape component connectors by name.
-    """
-
-    _by_name = {}
-
-    @classmethod
-    def get(cls, name):
-        """Get the connector class for the given Landscape component.
-
-        @param name: Name of the Landscape component we want to connect to, for
-           instance C{monitor} or C{manager}.
-        """
-        return cls._by_name[name]
-
-    @classmethod
-    def register(cls, connector_class):
-        """Register a connector for a Landscape component.
-
-        @param connector_class: A sub-class of L{ComponentConnector}
-            that can be used to connect to a certain component.
-        """
-        cls._by_name[connector_class.component.name] = connector_class
-
-
 def _get_socket_path(component, config):
     return os.path.join(config.sockets_path, component.name + ".sock")
 
=== modified file 'landscape/broker/amp.py'
--- landscape/broker/amp.py	2013-05-07 22:47:06 +0000
+++ landscape/broker/amp.py	2013-05-16 16:41:25 +0000
@@ -1,8 +1,7 @@
 from twisted.internet.defer import maybeDeferred, execute, succeed
 
 from landscape.lib.amp import RemoteObject, MethodCallArgument
-from landscape.amp import (ComponentConnector, ComponentsRegistry,
-                           get_remote_methods)
+from landscape.amp import ComponentConnector, get_remote_methods
 from landscape.broker.server import BrokerServer
 from landscape.broker.client import BrokerClient
 from landscape.monitor.monitor import Monitor
@@ -90,7 +89,15 @@
 
     component = Manager
 
-ComponentsRegistry.register(RemoteBrokerConnector)
-ComponentsRegistry.register(RemoteClientConnector)
-ComponentsRegistry.register(RemoteMonitorConnector)
-ComponentsRegistry.register(RemoteManagerConnector)
+
+def get_component_registry():
+    """Get a mapping of component name to connectors, for all components."""
+    all_connectors = [
+        RemoteBrokerConnector,
+        RemoteClientConnector,
+        RemoteMonitorConnector,
+        RemoteManagerConnector
+    ]
+    return dict(
+        (connector.component.name, connector)
+        for connector in all_connectors)
 
=== modified file 'landscape/broker/server.py'
--- landscape/broker/server.py	2013-05-14 22:57:15 +0000
+++ landscape/broker/server.py	2013-05-16 16:41:25 +0000
@@ -3,9 +3,8 @@
 from twisted.internet.defer import Deferred
 
 from landscape.lib.twisted_util import gather_results
-from landscape.amp import ComponentsRegistry
+from landscape.amp import remote
 from landscape.manager.manager import FAILED
-from landscape.amp import remote
 
 
 def event(method):
@@ -38,10 +37,11 @@
     @param message_store: The broker's L{MessageStore}.
     """
     name = "broker"
-    connectors_registry = ComponentsRegistry
 
     def __init__(self, config, reactor, exchange, registration,
                  message_store, pinger):
+        from landscape.broker.amp import get_component_registry
+        self.connectors_registry = get_component_registry()
         self._config = config
         self._reactor = reactor
         self._exchanger = exchange