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