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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
