Change in vdsm[master]: storage: Add resize map support in multipath.py

nsoffer at redhat.com nsoffer at redhat.com
Sun May 10 12:28:19 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: storage: Add resize map support in multipath.py
......................................................................


Patch Set 10:

(4 comments)

https://gerrit.ovirt.org/#/c/40468/10/vdsm/storage/multipath.py
File vdsm/storage/multipath.py:

Line 97:     name = devicemapper.getDmId(guid)
Line 98:     slaves = [(slave, getDeviceSize(slave))
Line 99:               for slave in devicemapper.getSlaves(name)]
Line 100: 
Line 101:     if(not _validate_slaves(slaves, guid)):
Don't add parenthesis around if condition - this is needed only if the condition span multiple lines.
Line 102:         return False
Line 103: 
Line 104:     name = devicemapper.getDmId(guid)
Line 105:     map_size = getDeviceSize(name)


Line 112:     supervdsm.getProxy().resizeMap(name)
Line 113:     return True
Line 114: 
Line 115: 
Line 116: def _validate_slaves(slaves, guid):
We should use a name that make the caller code more clear:

    if not _need_resize(guid, slaves):
        return False

I'm not sure that adding this helper is better than the previous version. For example, passing the guid which is needed for log messages or exceptions which are related to the context of the caller tell us that this code is not in the right place.

Consider in-lining it back in resize_if_needed.
Line 117: 
Line 118:     if len(slaves) == 0:
Line 119:         log.warning("Map %r has no slaves" % guid)
Line 120:         return False


Line 113:     return True
Line 114: 
Line 115: 
Line 116: def _validate_slaves(slaves, guid):
Line 117: 
This empty line is unneeded.
Line 118:     if len(slaves) == 0:
Line 119:         log.warning("Map %r has no slaves" % guid)
Line 120:         return False
Line 121: 


Line 116: def _validate_slaves(slaves, guid):
Line 117: 
Line 118:     if len(slaves) == 0:
Line 119:         log.warning("Map %r has no slaves" % guid)
Line 120:         return False
Not sure that moving the slave length check here is helping. We don't need a helper for checking size of a list.
Line 121: 
Line 122:     if len(set(size for slave, size in slaves)) != 1:
Line 123:             raise Error("Map %r slaves size differ %s" % (guid, slaves))
Line 124: 


-- 
To view, visit https://gerrit.ovirt.org/40468
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Fred Rolland <frolland at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list