Nir Soffer has posted comments on this change. Change subject: virt: stats: make compute_latency more robust ......................................................................
Patch Set 10: (2 comments) https://gerrit.ovirt.org/#/c/50593/10/vdsm/virt/vmstats.py File vdsm/virt/vmstats.py: Line 332: elapsed_time = ( Line 333: last_sample['block.%d.%s.times' % (last_index, mode)] - Line 334: first_sample['block.%d.%s.times' % (first_index, mode)] Line 335: ) Line 336: return elapsed_time / operations I would do it like this: last = "block.%d.%s" % (last_index, mode) first = "block.%d.%s" % (first_index, mode) operations = last_sample[last + ".reqs] - first_sample[first + ".reqs"] elapsed = last_sample[last + ".times"] - first_sample[first + ".times"] return elapsed / operations Do you see any issue with this? Line 337: Line 338: Line 339: def _disk_latency(first_sample, first_index, last_sample, last_index): Line 340: Line 348: last_sample, last_index, mode) Line 349: except KeyError: Line 350: pass Line 351: else: Line 352: stats[name] = str(value) Much nicer, but why not use continue to eliminate the else? else is useful when we have something to do in that case. Line 353: Line 354: return stats Line 355: Line 356: -- To view, visit https://gerrit.ovirt.org/50593 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie4670309d48bd65a3afaba8124407598b76d808d Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
