Martin Sivák has posted comments on this change. Change subject: New hypervisor interface for remote VDSM over XML-RPC with stats caching ......................................................................
Patch Set 5: (3 comments) https://gerrit.ovirt.org/#/c/41570/5/mom/HypervisorInterfaces/vdsmxmlrpcInterface.py File mom/HypervisorInterfaces/vdsmxmlrpcInterface.py: Line 72: ret = self.vdsm_api.getAllVmStats() Line 73: self._check_status(ret) Line 74: except vdsmException, e: Line 75: e.handle_exception() Line 76: return vms > maybe (feel free to ignore): I prefer to use else only when it is directly related to the exception to make things readable (less nesting). Line 77: Line 78: for vm in ret['statsList']: Line 79: vms[vm['vmId']] = vm Line 80: return vms Line 81: Line 82: def getVmStats(self, vmId): Line 83: return self.getAllVmStats()[vmId] Line 84: Line 85: def _vmIsRunning(self, vm): > I'd inline this, but no big deal. Good idea. Line 86: if vm['status'] == 'Up': Line 87: return True Line 88: else: Line 89: return False Line 162: def getVmInfo(self, id): Line 163: data = {} Line 164: data['uuid'] = id Line 165: data['pid'] = self.getVmPid(id) Line 166: data['name'] = self.getVmName(id) > AFAIK we use getVm{Pid,Name} only here, so I'd rather do getVmStats() once Done. Line 167: if None in data.values(): Line 168: return None Line 169: return data Line 170: -- To view, visit https://gerrit.ovirt.org/41570 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I314f39f9020ee257827145a57f585e7921a913e3 Gerrit-PatchSet: 5 Gerrit-Project: mom Gerrit-Branch: master Gerrit-Owner: Martin Sivák <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
