Code review comment for lp:~radix/landscape-client/fix-landscape-client-startup

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

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

« Back to merge proposal