Adam Litke has posted comments on this change. Change subject: Live Merge: Extend internal block volumes during merge ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/31268/2/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2065: def _getLiveMergeExtendCandidates(self): Line 2066: ret = {} Line 2067: Line 2068: # The common case is that there are no active jobs. Line 2069: if len(self.conf['_blockJobs'].values()) == 0: > any reason to not simply use Nope, we can write it that way. Line 2070: return ret Line 2071: Line 2072: drives = [d for d in self.getDiskDevices() Line 2073: if isVdsmImage(d) and bool(d.blockDev)] Line 2069: if len(self.conf['_blockJobs'].values()) == 0: Line 2070: return ret Line 2071: Line 2072: drives = [d for d in self.getDiskDevices() Line 2073: if isVdsmImage(d) and bool(d.blockDev)] > why explicit bool() ? I personally don't like that d.blockDev semantics are that it's False for non block devices and a string (the device) otherwise. It's a matter of style for me to prefer the cast when such an asymetric property exists. That being said, I am happy to remove the explicit cast if it's giving you heartburn. Line 2074: allChains = self._driveGetActualVolumeChain(drives).values() Line 2075: watermarks = {} Line 2076: for chain in allChains: Line 2077: for vol in chain: Line 2074: allChains = self._driveGetActualVolumeChain(drives).values() Line 2075: watermarks = {} Line 2076: for chain in allChains: Line 2077: for vol in chain: Line 2078: watermarks[vol['id']] = vol['allocation'] > what about heh, I've never nested list comprehensions in this way. It's 3 lines instead of 4 so what the heck. I'll switch it. Line 2079: Line 2080: for job in self.conf['_blockJobs'].values(): Line 2081: try: Line 2082: drive = self._findDriveByUUIDs(job['disk']) Line 5488: break Line 5489: sourceAttr = ('file', 'dev')[drive.blockDev] Line 5490: path = sourceXML.getAttribute(sourceAttr) Line 5491: info = {'id': pathToVolID(drive, path), 'path': path} Line 5492: if bool(drive.blockDev): > same as per line 2073 ok. Line 5493: allocXML = findElement(diskXML, 'allocation') Line 5494: if not allocXML: Line 5495: self.log.debug("<allocation/> missing from backing " Line 5496: "chain for block device %s", drive.name) -- To view, visit http://gerrit.ovirt.org/31268 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a9e0ebdb9c42df713c40e0fc5782945eb7228a8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
