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