Change in vdsm[master]: Refactor metadata operations
nsoffer at redhat.com
nsoffer at redhat.com
Fri Apr 15 21:29:38 UTC 2016
Nir Soffer has posted comments on this change.
Change subject: Refactor metadata operations
......................................................................
Patch Set 2:
(8 comments)
https://gerrit.ovirt.org/#/c/52671/2/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:
Line 257: volume.type2name(volume.LEAF_VOL),
Line 258: disk_type,
Line 259: desc,
Line 260: volume.LEGAL_VOL)
Line 261: self._oop.writeLines(self.meta_volatile_path, meta.format())
You are passing string, and this function seems to expect lines. Hopefully we have another function writing data - if not, we can add it.
Line 262:
Line 263: def _create_lease_file(self):
Line 264: if self.sd_manifest.hasVolumeLeases():
Line 265: meta_id = (self.volume_path,)
https://gerrit.ovirt.org/#/c/52671/2/vdsm/storage/volume.py
File vdsm/storage/volume.py:
Line 174:
Line 175: def __init__(self, sd_id, img_id, parent_vol_id, size, vol_format,
Line 176: prealloc, vol_type, disk_type, description="",
Line 177: legality=ILLEGAL_VOL):
Line 178: self.sd_id = str(sd_id)
Why convert to string? it should be a string.
Line 179: self.img_id = str(img_id)
Line 180: self.parent_vol_id = str(parent_vol_id)
Line 181: self.size = int(size)
Line 182: self.vol_format = str(vol_format)
Line 177: legality=ILLEGAL_VOL):
Line 178: self.sd_id = str(sd_id)
Line 179: self.img_id = str(img_id)
Line 180: self.parent_vol_id = str(parent_vol_id)
Line 181: self.size = int(size)
Why not require integer value?
We should require the correct type in __init__. We will convert to string when formating, and when reading from storage, back to the correct type.
Line 182: self.vol_format = str(vol_format)
Line 183: self.prealloc = str(prealloc)
Line 184: self.vol_type = str(vol_type)
Line 185: self.disk_type = str(disk_type)
Line 185: self.disk_type = str(disk_type)
Line 186: self.description = description
Line 187: self.legality = str(legality)
Line 188: self.ctime = int(time.time())
Line 189: self.mtime = 0
There is no point in keeping constant value as instance variable. We can use a read only property instead:
@property
def mtime(self):
return 0
And if we don't access this in any code, just hardcode the value in the info().
Line 190:
Line 191: @property
Line 192: def description(self):
Line 193: return self._description
Line 193: return self._description
Line 194:
Line 195: @description.setter
Line 196: def description(self, desc):
Line 197: self._description = VolumeManifest.validateDescription(desc)
Can we move the validation code to this class and use this class in VolumeManifest?
Line 198:
Line 199: def format(self):
Line 200: """
Line 201: Format metadata string in storage format.
Line 201: Format metadata string in storage format.
Line 202:
Line 203: Raises MetadataOverflowError if formatted metadata is too long.
Line 204: """
Line 205: lines = ["%s=%s\n" % (key.strip(), str(value).strip())
Why strip? we are using info(), which should not return extra whitespace.
We also do not need to convert to string, as "%s" will convert any type to string.
Line 206: for key, value in self.info().iteritems()]
Line 207: lines.append("EOF\n")
Line 208: data = "".join(lines)
Line 209: if len(data) > METADATA_SIZE:
Line 202:
Line 203: Raises MetadataOverflowError if formatted metadata is too long.
Line 204: """
Line 205: lines = ["%s=%s\n" % (key.strip(), str(value).strip())
Line 206: for key, value in self.info().iteritems()]
iteritems will not work in python3, the portable solution is:
for key, value in six.iteritems(self.info())
Line 207: lines.append("EOF\n")
Line 208: data = "".join(lines)
Line 209: if len(data) > METADATA_SIZE:
Line 210: raise se.MetadataOverflowError(data)
Line 209: if len(data) > METADATA_SIZE:
Line 210: raise se.MetadataOverflowError(data)
Line 211: return data
Line 212:
Line 213: def info(self):
Do we use this dict in old code?
If not, we don't have to convert value to string if they are not strings.
Line 214: """
Line 215: Return metadata in dictionary format
Line 216: """
Line 217: return {
--
To view, visit https://gerrit.ovirt.org/52671
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fee56b30c13a3c1cede8489338ed60f4e1d5eab
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list