Code review comment for lp:~sandy-walsh/nova/zones3

Revision history for this message
Rick Harris (rconradharris) wrote :

Overall this patch looks really good Sandy. Nice tests, and some really
helpful comments in there (as usual).

Generally speaking, I think a few whitespaces cleanups would make a make this
patch even better (in particular lining up arg-lists). Not a huge deal,
obviously, but worth mentioning.

In terms of design and implementation, I think it looks good. This matches up
with what we whiteboarded (AFAICT), so I'm all for it.

Specifics
=========

> 10 +from nova import log as logging

Imported but not used.

> 35 + for cap in caps:
> 36 + key_values = cap.split('=')
> 37 + zone[key_values[0]] = key_values[1]

Might be slightly easier to read with tuple-unpacking (also, passing 1 to
split in case the value has a '=' in it):

    for cap in caps:
        key, value = cap.split('=', 1)
        zone[key] = value

> 62 + super(ComputeManager, self).__init__(service_name="compute",
> 63 + *args, **kwargs)

Might be better if args aligned:

    super(ComputeManager, self).__init__(service_name="compute",
                                         *args, **kwargs)
> 77 +DEFINE_list('zone_capabilities',
> 78 + ['hypervisor=xenserver;kvm', 'os=linux;windows'],
> 79 + 'Key/Multi-value list representng capabilities of this zone')

Same as above:

    DEFINE_list('zone_capabilities',
                ['hypervisor=xenserver;kvm', 'os=linux;windows'],
                'Key/Multi-value list representng capabilities of this zone')

> 88 +from nova import log as logging

Might be worth using the LOG pattern here to get more specific logging:

    LOG = logging.getLogger('nova.manager')

> 106 + update_service_capabilities is called with non-None values."""
> 107 + def __init__(self, host=None, db_driver=None, service_name="undefined"):

Spacing. Might be better as:

       update_service_capabilities is called with non-None values.
    """

    def __init__(self, host=None, db_driver=None, service_name="undefined"):

> 313 + params=dict(service=service))

Couple spaces ---> thataway (to line up with open paren +1)

> 209 + LOG.info(_("Created '%(exchange)s' fanout exchange "
> 210 + "with '%(key)s' routing key"),
> 211 + dict(exchange=self.exchange, key=self.routing_key))

Could use a bit of formatting work:

    LOG.info(_("Created '%(exchange)s' fanout exchange "
               "with '%(key)s' routing key"),
             dict(exchange=self.exchange, key=self.routing_key))

Although, this might be personal preference on my part, so, if that change
doesn't look good to you, please feel free to ignore! :-)

> 638 + svc10_a=(99, 99), svc10_b=(99, 99)))

Spacing, a couple of spaces <---- thattaway.

review: Needs Fixing

« Back to merge proposal