Change in vdsm[master]: VolumeMetadata: Introduce class structure

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 8 23:17:55 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: VolumeMetadata: Introduce class structure
......................................................................


Patch Set 3:

(9 comments)

https://gerrit.ovirt.org/#/c/41844/3/vdsm/storage/blockVolume.py
File vdsm/storage/blockVolume.py:

Line 71: 
Line 72: class BlockVolumeMetadata(volume.VolumeMetadata):
Line 73:     def __init__(self, repoPath, sdUUID, imgUUID, volUUID):
Line 74:         volume.VolumeMetadata.__init__(self, repoPath, sdUUID, imgUUID,
Line 75:                                        volUUID)
Unneeded, see FileVolumeMetadata
Line 76: 
Line 77: 
Line 78: class BlockVolume(volume.Volume):
Line 79:     """ Actually represents a single volume (i.e. part of virtual disk).


Line 77: 
Line 78: class BlockVolume(volume.Volume):
Line 79:     """ Actually represents a single volume (i.e. part of virtual disk).
Line 80:     """
Line 81:     MetadataClass = BlockVolumeMetadata
metadataClass?
Line 82: 
Line 83:     def __init__(self, repoPath, sdUUID, imgUUID, volUUID):
Line 84:         md = self.MetadataClass(repoPath, sdUUID, imgUUID, volUUID)
Line 85:         self.metaoff = None


Line 82: 
Line 83:     def __init__(self, repoPath, sdUUID, imgUUID, volUUID):
Line 84:         md = self.MetadataClass(repoPath, sdUUID, imgUUID, volUUID)
Line 85:         self.metaoff = None
Line 86:         volume.Volume.__init__(self, md, repoPath, sdUUID, imgUUID, volUUID)
Same, Volume.__init__ needs only md
Line 87:         self.lvmActivationNamespace = sd.getNamespace(self.sdUUID,
Line 88:                                                       LVM_ACTIVATION_NAMESPACE)
Line 89: 
Line 90:     def validate(self):


https://gerrit.ovirt.org/#/c/41844/3/vdsm/storage/fileVolume.py
File vdsm/storage/fileVolume.py:

Line 53: 
Line 54: class FileVolumeMetadata(volume.VolumeMetadata):
Line 55:     def __init__(self, repoPath, sdUUID, imgUUID, volUUID):
Line 56:         volume.VolumeMetadata.__init__(self, repoPath, sdUUID, imgUUID,
Line 57:                                        volUUID)
This __init__ is unneeded, we did not change the signature and we are not doing anything.
Line 58: 
Line 59: 
Line 60: class FileVolume(volume.Volume):
Line 61:     """ Actually represents a single volume (i.e. part of virtual disk).


Line 59: 
Line 60: class FileVolume(volume.Volume):
Line 61:     """ Actually represents a single volume (i.e. part of virtual disk).
Line 62:     """
Line 63:     MetadataClass = FileVolumeMetadata
metadataClass?
Line 64: 
Line 65:     def __init__(self, repoPath, sdUUID, imgUUID, volUUID):
Line 66:         md = self.MetadataClass(repoPath, sdUUID, imgUUID, volUUID)
Line 67:         volume.Volume.__init__(self, md, repoPath, sdUUID, imgUUID, volUUID)


Line 63:     MetadataClass = FileVolumeMetadata
Line 64: 
Line 65:     def __init__(self, repoPath, sdUUID, imgUUID, volUUID):
Line 66:         md = self.MetadataClass(repoPath, sdUUID, imgUUID, volUUID)
Line 67:         volume.Volume.__init__(self, md, repoPath, sdUUID, imgUUID, volUUID)
We should remove repoPath, sdUUID, imgUUID, volUUID from Volume.__init__, like you did with StorageDomain.
Line 68: 
Line 69:     @property
Line 70:     def oop(self):
Line 71:         return oop.getProcessPool(self.sdUUID)


https://gerrit.ovirt.org/#/c/41844/3/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 134
Line 135
Line 136
Line 137
Line 138
Why duplicate these attributes in Volume and VolumeMetadata?


Line 138
Line 139
Line 140
Line 141
Line 142
This validation should be part of VolumeMetadata.__init__


Line 142: class Volume(object):
Line 143:     log = logging.getLogger('Storage.Volume')
Line 144: 
Line 145:     def __init__(self, md, repoPath, sdUUID, imgUUID, volUUID):
Line 146:         self.md = md
Why not metadata? and why public? we don't want other objects to peek into volume.md.things.
Line 147:         self.repoPath = repoPath
Line 148:         self.sdUUID = sdUUID
Line 149:         self.imgUUID = imgUUID
Line 150:         self.volUUID = volUUID


-- 
To view, visit https://gerrit.ovirt.org/41844
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0552bea23b04b9e58e5d2cc7e016103d03194d1a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list