Dan Kenigsberg has posted comments on this change. Change subject: virt: stats: make disk_rate more robust ......................................................................
Patch Set 11: (3 comments) https://gerrit.ovirt.org/#/c/48493/11/vdsm/virt/vmstats.py File vdsm/virt/vmstats.py: Line 304: return stats Line 305: Line 306: Line 307: def _disk_rate(first_sample, first_index, last_sample, last_index, interval): Line 308: def compute_rate(mode): > Using inner function is required only when you want to use the outer functi the down side is that defining it outside clutters the module name space. but since _disk_rate is called so often, there might be a measurable benefit of defining the helper function only once. Line 309: return str( Line 310: (last_sample['block.%d.%s.bytes' % (last_index, mode)] - Line 311: first_sample['block.%d.%s.bytes' % (first_index, mode)]) / Line 312: interval Line 309: return str( Line 310: (last_sample['block.%d.%s.bytes' % (last_index, mode)] - Line 311: first_sample['block.%d.%s.bytes' % (first_index, mode)]) / Line 312: interval Line 313: ) > Don't use this style, use temporary variables to make the code more clear: I love self documenting code, but in this specific case I see no benefit in assigning the throw-away variables. their purpose is clear even without an explicit name. Line 314: Line 315: stats = {} Line 316: Line 317: def add_rate(name, mode): Line 313: ) Line 314: Line 315: stats = {} Line 316: Line 317: def add_rate(name, mode): > No need for inner helper function, you can inline it here using a loop: +1 Line 318: try: Line 319: stats[name] = compute_rate(mode) Line 320: except KeyError: Line 321: pass -- To view, visit https://gerrit.ovirt.org/48493 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib2900e73161932f7fbecd9d7bfaba36dc1e84e19 Gerrit-PatchSet: 11 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
