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 <froll...@redhat.com> Gerrit-Reviewer: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
