Change in vdsm[master]: Refactor metadata operations
nsoffer at redhat.com
nsoffer at redhat.com
Wed Apr 20 21:30:55 UTC 2016
Nir Soffer has posted comments on this change.
Change subject: Refactor metadata operations
......................................................................
Patch Set 4: Code-Review-1
(5 comments)
Looks good, but see the comments.
Lets have also another review.
https://gerrit.ovirt.org/#/c/52671/4/tests/storage_volume_metadata_test.py
File tests/storage_volume_metadata_test.py:
Line 35: vol_format=volume.type2name(volume.RAW_FORMAT),
Line 36: alloc_policy=volume.type2name(volume.SPARSE_VOL),
Line 37: parental_status=volume.type2name(volume.LEAF_VOL),
Line 38: disk_type=str(image.SYSTEM_DISK_TYPE),
Line 39: description="", legality=volume.LEGAL_VOL):
Very hard to read like this.
Line 40: return dict(
Line 41: sd_id=sd_id or str(uuid.uuid4()),
Line 42: img_id=img_id or str(uuid.uuid4()),
Line 43: parent_vol_id=parent_vol_id or str(uuid.uuid4()),
Line 42: img_id=img_id or str(uuid.uuid4()),
Line 43: parent_vol_id=parent_vol_id or str(uuid.uuid4()),
Line 44: size=size, vol_format=vol_format, alloc_policy=alloc_policy,
Line 45: parental_status=parental_status, disk_type=disk_type,
Line 46: description=description, legality=legality)
Same.
Lets make it simpler and more readable:
def make_params(**kw):
res = {
"key": "value",
...
}
res.update(kw)
return res
Line 47:
Line 48:
Line 49: @expandPermutations
Line 50: class VolumeMetadataTests(VdsmTestCase):
Line 71: self.assertEqual(expected, info)
Line 72:
Line 73: def test_format(self):
Line 74: params = get_params()
Line 75: params['ctime'] = FAKE_TIME
If you take my **kw suggestion, we can do now:
params = make_params(ctime=FAKE_TIME)
Line 76: expected = textwrap.dedent("""\
Line 77: CTIME=%(ctime)s
Line 78: DESCRIPTION=%(description)s
Line 79: DISKTYPE=%(disk_type)s
Line 103:
Line 104: @permutations([['size'], ['ctime'], ['mtime']])
Line 105: def test_int_params_str_raises(self, param):
Line 106: params = get_params()
Line 107: params[param] = 'not_an_int'
If you take my **kw suggestion, we can do now:
params = make_params(param='not an int')
https://gerrit.ovirt.org/#/c/52671/4/vdsm/storage/volume.py
File vdsm/storage/volume.py:
Line 183: self.parent_vol_id = parent_vol_id
Line 184: self.size = size
Line 185: self.vol_format = vol_format
Line 186: self.alloc_policy = alloc_policy
Line 187: self.parental_status = parental_status
This kind of work for leaf or internal, but what about shared?
Lets keep the previous version (vol_type)?
Line 188: self.disk_type = disk_type
Line 189: self.description = description
Line 190: self.legality = legality
Line 191: self.ctime = int(time.time()) if ctime is None else ctime
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: 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