Change in vdsm[master]: BZ#788640 - Add [block|file]SD.getAllVolumes
danken at redhat.com
danken at redhat.com
Sun May 13 19:15:48 UTC 2012
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#788640 - Add [block|file]SD.getAllVolumes
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(7 inline comments)
....................................................
File vdsm/storage/blockSD.py
Line 127: def _getVolsTree(sdUUID):
Return dict {volUUID: (name, image, parent)}
Line 151:
imgUUIDs is a list of all images dependant on volUUID. For template base volumes, the first image is the template's image. For other volumes, there is just a single imageUUID.
Line 161: if vImg not in res[vName]['imgs']:
I'm lost. And I've passed Graph Theory 101.
Why not put the self-image first for EVERYTHING, when initializing res[vName] above?
Either way, I would be much happier if you add a unit test for this function: mock _getVolsTree() to return an interesting set of BlockSdVol tuples, and verify that getAllVolumes() generates the needed dict.
Line 168: return dict((k, sd.ImgsPar(tuple(v['imgs']), v['parent'])) for k, v in res.iteritems())
new code should have shorter lines.
....................................................
File vdsm/storage/fileSD.py
Line 63: sdUUID = len(os.path.normpath(config.get('irs', 'repository')).split('/')) + sdUUIDPos
no idea why this change was so urgent to do in this patch.
Line 261: volMetaPaths = self.oop.glob.glob(volMetaPattern)
here, too, I'd feel more comfortable if you add a unit test, supplying a list of paths and validating the output.
Line 279: if len(tuple(vPath for vPath in volMetaPaths
Just like Ayal, I have no idea why you bother to convert a temporary list to a tuple. it is not a returned object. The conversion of line 288 is comprehensible.
More importantly, this line takes you into O(volMetaPaths**2). You could do this in O(volMetaPaths) using itertools.groupby() with keyfunc=imgOfMetaPath
--
To view, visit http://gerrit.ovirt.org/3462
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7eccf5ca100bd354aa09208ca60bb112fb697063
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Haim Ateya <hateya at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
More information about the vdsm-patches
mailing list