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

Revision history for this message
Jay Pipes (jaypipes) wrote :

Hi!

Good second round of coding! :)

72 + for item in items:
73 + item['api_url'] = item['api_url'].replace('\\/', '/')

Are there a lot of URLs with \/ in them?

76 + if len(items) == 0:

Could just shorten the above to: if not items:

106 +DEFINE_string('zone_capabilities', 'xen, linux',
107 + 'comma-delimited list of tags which represent boolean'
108 + ' capabilities of this zone')

I was under the impression that we wanted to store these kind of things in the database, no? I think a flag of boolean-like capabilities is a bit limiting.

277 + def _poll_zones(self, context):
278 + """Try to connect to each child zone and get update."""
279 + green_pool = GreenPool()
280 + green_pool.imap(_poll_zone, self.zone_states.values())

Would it be a bit more efficient to have the ZoneManager initialize a GreenPool in its constructure and have ZoneManager._poll_zones() simply use that pool?

148 +# Copyright (c) 2010 Openstack, LLC.

s/2010/2011 :)

286 + logging.debug("Updating zone cache from db.")

Missed an i18n above

Other than those little nits, looks good. :)

I encourage you to add a bit of overview documentation to the doc/source/ RST files when you get a chance in a future commit.

-jay

review: Needs Fixing

« Back to merge proposal