Change in vdsm[master]: virt: stats: make compute_latency more robust
mpolednik at redhat.com
mpolednik at redhat.com
Thu Jan 21 13:46:41 UTC 2016
Martin Polednik has posted comments on this change.
Change subject: virt: stats: make compute_latency more robust
......................................................................
Patch Set 11: Code-Review-1
(4 comments)
Some naming issues and inconsistencies. +1 for the improvement, mild -1 for attention but I wouldn't block on it unless other reviewers find the same issues annoying.
https://gerrit.ovirt.org/#/c/50593/11/vdsm/virt/vmstats.py
File vdsm/virt/vmstats.py:
Line 320: return stats
Line 321:
Line 322:
Line 323: def _compute_latency(first_sample, first_index,
Line 324: last_sample, last_index, mode):
If you insist on moving the function out of _disk_latency scope the name becomes too generic. _compute_disk_latency would make a bit more sense as we cannot use it to compute *any* kind of latency.
Idea: is it possible to make the function generic enough to be used in some other computations?
Line 325: last = "block.%d.%s" % (last_index, mode)
Line 326: first = "block.%d.%s" % (first_index, mode)
Line 327:
Line 328: operations = last_sample[last + ".reqs"] - first_sample[first + ".reqs"]
Line 322:
Line 323: def _compute_latency(first_sample, first_index,
Line 324: last_sample, last_index, mode):
Line 325: last = "block.%d.%s" % (last_index, mode)
Line 326: first = "block.%d.%s" % (first_index, mode)
I'd love to see first_key and last_key, even for the price of having to break up the 2 compute lines. Or rename them in 48493 for the sake of consistence. +1 on the ltype->mode though!
Line 327:
Line 328: operations = last_sample[last + ".reqs"] - first_sample[first + ".reqs"]
Line 329: if not operations:
Line 330: return 0
Line 324: last_sample, last_index, mode):
Line 325: last = "block.%d.%s" % (last_index, mode)
Line 326: first = "block.%d.%s" % (first_index, mode)
Line 327:
Line 328: operations = last_sample[last + ".reqs"] - first_sample[first + ".reqs"]
See what I said in 48493 - this really deserves some more documentation (doesn't have to be in this patch).
Line 329: if not operations:
Line 330: return 0
Line 331: elapsed = last_sample[last + ".times"] - first_sample[first + ".times"]
Line 332: return elapsed / operations
Line 327:
Line 328: operations = last_sample[last + ".reqs"] - first_sample[first + ".reqs"]
Line 329: if not operations:
Line 330: return 0
Line 331: elapsed = last_sample[last + ".times"] - first_sample[first + ".times"]
I don't understand the name change elapsed_time -> elapsed. Does it measure time? Operations? What are we actually calculating here?
Line 332: return elapsed / operations
Line 333:
Line 334:
Line 335: def _disk_latency(first_sample, first_index, last_sample, last_index):
--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list