Change in vdsm[master]: Refactor metadata operations

nsoffer at redhat.com nsoffer at redhat.com
Thu Apr 21 14:45:11 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: Refactor metadata operations
......................................................................


Patch Set 4:

(3 comments)

https://gerrit.ovirt.org/#/c/52671/4/tests/storage_volume_metadata_test.py
File tests/storage_volume_metadata_test.py:

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'
> Actually it's not as clean because param is a variable not the key name its
Right

 **{param: 'not an int'} will do in this case params[param] = 'not an int' is nicer.


https://gerrit.ovirt.org/#/c/52671/4/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 458
Line 459
Line 460
Line 461
Line 462
This use now VolumeMedadata to write the description, right?

Since it already validate and truncate the description, we can remove the extra validation here, so this method would be just a call to the setMetaParam with the correct key.


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
> Shared is a different kind of parental status in my opinion.  To me, parent
This is a common infrastructure that will be used everywhere, so it is not about personal preferences.

By using new names instead of the names in the old api and the storage format, we make the code harder to understand. How can developer understand which code is writing VOLTYPE=INTERNAL? 

If we keep the old names, this is very clear.
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: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby 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