Martin Polednik has posted comments on this change. Change subject: virt: sampling: extract disk latency calculation ......................................................................
Patch Set 4: Code-Review-1 (2 comments) -1 for visibility, not critical to me http://gerrit.ovirt.org/#/c/29952/4/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 570: try: Line 571: if (sInfo and vmDrive.name in sInfo and Line 572: eInfo and vmDrive.name in eInfo): Line 573: dStats = self._calcDiskLatency(vmDrive, sInfo, eInfo) Line 574: except (KeyError, TypeError): Have you thought of adding AttributeError, NameError and removing if block above? Line 575: self._log.exception("Disk %s latency not available", Line 576: vmDrive.name) Line 577: Line 578: stats[vmDrive.name].update(dStats) Line 596: sData['flush_operations']) Line 597: else (eData['flush_total_times'] - Line 598: sData['flush_total_times']) / Line 599: (eData['flush_operations'] - Line 600: sData['flush_operations'])) I'd consider iterating over some composed data structure in order to reduce the amount of copy-pastiness in this code latency = {} for type in ('rd', 'wr', 'flush'): (0 if not eData[type + '_operations'].... latency[type] Line 601: return {'readLatency': str(readLatency), Line 602: 'writeLatency': str(writeLatency), Line 603: 'flushLatency': str(flushLatency)} Line 604: -- To view, visit http://gerrit.ovirt.org/29952 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3f78e2a990aefe6095dd4ff54e21ab40006c0713 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
