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:
--
To view, visit
https://gerrit.ovirt.org/48097
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes