Nir Soffer has posted comments on this change.
Change subject: Introduce VolumeArtifacts ......................................................................
Patch Set 1: Code-Review-1
(4 comments)
I like this, needs more time to review properly.
https://gerrit.ovirt.org/#/c/48097/1/vdsm/storage/fileVolume.py File vdsm/storage/fileVolume.py:
Line 54: class FileVolumeArtifacts(volume.VolumeArtifacts): Line 55: def __init__(self, domain_manifest, img_id, vol_id): Line 56: super(FileVolumeArtifacts, self).__init__( Line 57: domain_manifest, img_id, vol_id) Line 58: self._oop = domain_manifest.oop Why do you want to copy the manifest oop here?
Define a property to access it instead. Otherwise we will have new entry in each instance dict for this. Line 59: self._artifacts_path = None Line 60: Line 61: def create(self, size, vol_format, disk_type, desc, parent_vol_id=None): Line 62: self._create_artifacts_path()
https://gerrit.ovirt.org/#/c/48097/1/vdsm/storage/volume.py File vdsm/storage/volume.py:
Line 164: TYPE_PATH = "path" Line 165: TYPE_NETWORK = "network" Line 166: Line 167: Line 168: class VolumeArtifactsNotFound(Exception): Can't we use existing exceptions in storage_exceptions? Line 169: pass Line 170: Line 171: Line 172: class CannotCreateVolumeArtifacts(Exception):
Line 172: class CannotCreateVolumeArtifacts(Exception): Line 173: pass Line 174: Line 175: Line 176: class VolumeArtifacts(object): This looks like an interface - do we really want to keep instance variables here? Line 177: def __init__(self, domain_manifest, img_id, vol_id): Line 178: self.domain_manifest = domain_manifest Line 179: self.img_id = img_id Line 180: self.vol_id = vol_id
Line 177: def __init__(self, domain_manifest, img_id, vol_id): Line 178: self.domain_manifest = domain_manifest Line 179: self.img_id = img_id Line 180: self.vol_id = vol_id Line 181: self.log = logging.getLogger('Storage.VolumeArtifacts') log must be class attribute, otherwise you are creating new logger for each instance - these loggers are *never* deleted from logging. Line 182: Line 183: def create(self, size, vol_format, disk_type, desc, parent_vol_id=None): Line 184: raise NotImplementedError() Line 185: