Code review comment for lp:~morita-kazutaka/nova/snapshot-volume

Revision history for this message
Mark Washenberger (markwash) wrote :

I haven't really waded into the volume stuff yet, but I thought I'd give a few comments from a first pass.

Minor nits:
- you should bump your database migration version up to the latest in trunk + 1
- we recently made pylint a little happier with our migrations by switching from

265 +from sqlalchemy import *
266 +from migrate import *

to

from sqlalchemy import Column, Table # ...

you might want to change your branch to do the same.

- I see you've added a sensible test to the database api functions you added. Is it possible to add some tests to the ec2 api functionality? What about the volume api and manager functionality?

- There is a blueprint coming down the pipe to get rid of database messaging. I wonder if it would be possible to adopt something along the lines of that approach in this merge. The part that concerns me slightly is the fact that both the volume api and volume manager are writing to the database. Perhaps more information could be added to the rpc cast so that the db writes in the volume api are unnecessary?

Again, this is my first foray into the volume code, so forgive me if I'm off-base. Thanks.

« Back to merge proposal