Change in vdsm[master]: StorageDomainManifest: introduce class heirarchy
nsoffer at redhat.com
nsoffer at redhat.com
Mon Jun 15 21:11:58 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: StorageDomainManifest: introduce class heirarchy
......................................................................
Patch Set 8:
(5 comments)
https://gerrit.ovirt.org/#/c/41993/8/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:
Line 417: manifestClass = BlockStorageDomainManifest
Line 418:
Line 419: def __init__(self, sdUUID):
Line 420: manifest = self.manifestClass(sdUUID)
Line 421: manifest.activate()
> The manifest is constructed with different args depending on its type. I c
Ok
Line 422: sd.StorageDomain.__init__(self, manifest)
Line 423: lvm.activateLVs(self.sdUUID, SPECIAL_LVS)
Line 424: self.metavol = lvm.lvPath(self.sdUUID, sd.METADATA)
Line 425:
https://gerrit.ovirt.org/#/c/41993/8/vdsm/storage/fileSD.py
File vdsm/storage/fileSD.py:
Line 163: if metadata is None:
Line 164: metadata = FileSDMetadata(self.metafile)
Line 165: self.replaceMetadata(metadata)
Line 166: if not self.oop.fileUtils.pathExists(self.metafile):
Line 167: raise se.StorageDomainMetadataNotFound(self.sdUUID, self.metafile)
> There are a few words we could use. Another I'd be okay with is 'bind'. T
Sounds better.
Line 168:
Line 169: def getReadDelay(self):
Line 170: stats = misc.readspeed(self.metafile, 4096)
Line 171: return stats['seconds']
Line 180:
Line 181: # We perform validation here since filesystem features are relevant to
Line 182: # construction of an actual Storage Domain. Direct users of
Line 183: # FileStorageDomainManifest should call this explicitly if required.
Line 184: validateFileSystemFeatures(manifest.sdUUID, manifest.mountpoint)
> This is a great idea but validateFileSystemFeatures is called in classmetho
Ok
Line 185: sd.StorageDomain.__init__(self, manifest)
Line 186: self.imageGarbageCollector()
Line 187: self._registerResourceNamespaces()
Line 188:
https://gerrit.ovirt.org/#/c/41993/8/vdsm/storage/sd.py
File vdsm/storage/sd.py:
Line 303: def oop(self):
Line 304: return oop.getProcessPool(self.sdUUID)
Line 305:
Line 306: def replaceMetadata(self, md):
Line 307: self._metadata = md
> What do you mean? We need to call it in replaceMetadata? If that's the ca
I mean that the super class should have either empty method or abstract method for stuff subclasses must/may implement. You want to look at this code and undestand what is the interface of a StorageDomainManifest without checking all subclasses.
Something like:
def bind(self):
""" Description """
raise NotImplementedError
Line 308:
Line 309:
Line 310: class StorageDomain(object):
Line 311: log = logging.getLogger("Storage.StorageDomain")
Line 320: 3: (clusterlock.SANLock, True),
Line 321: }
Line 322:
Line 323: def __init__(self, manifest):
Line 324: self._manifest = manifest
> See my comment about the manifest ctor args being different depending on th
Ok
Line 325: self._lock = threading.Lock()
Line 326: self.stat = None
Line 327: self._clusterLock = self._makeClusterLock()
Line 328:
--
To view, visit https://gerrit.ovirt.org/41993
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia275424a8d423722efb229a4d8545f035fe5fa51
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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