Change in vdsm[master]: sampling: extract SampleWindow class

fromani at redhat.com fromani at redhat.com
Mon Oct 27 14:19:00 UTC 2014


Francesco Romani has posted comments on this change.

Change subject: sampling: extract SampleWindow class
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.ovirt.org/#/c/33783/4/vdsm/virt/sampling.py
File vdsm/virt/sampling.py:

Line 305: 
Line 306: class SampleWindow(object):
Line 307:     """
Line 308:     Sliding window of samples. Automatically record
Line 309:     the timestamp of the given samples.
> Maybe something like: "Keep sliding window of samples"
Clear and simple. Done.
Line 310:     """
Line 311:     def __init__(self, size=0, timefn=time.time):
Line 312:         self._samples = deque(maxlen=size)
Line 313:         self._timefn = timefn


Line 307:     """
Line 308:     Sliding window of samples. Automatically record
Line 309:     the timestamp of the given samples.
Line 310:     """
Line 311:     def __init__(self, size=0, timefn=time.time):
> Is 0 a useful default size? Seems like an invalid value for sliding window.
This is a (bad) inheritance from advancedStatsFunction (I don't know yet why it is like this, most likely historical reason lost in time).

So, collections.deque already raises ValueError if fed with maxlen<0. We all agree size >= 2 makes fully sense, what is left out is the strange corner cases of size in (0, 1).
BTW, collections.deque accepts these, but I don't think this class should.

For the sake of making this patch minimal, because AdvancedStatsFunction depends on size=1 being supported, I'll make and enforce size >= 1. In followup patches we'll discuss the most appropiate value.
Line 312:         self._samples = deque(maxlen=size)
Line 313:         self._timefn = timefn
Line 314: 
Line 315:     def append(self, value):


Line 314: 
Line 315:     def append(self, value):
Line 316:         """
Line 317:         Adds a new sample, and automatically record
Line 318:         the timestamp of the new value.
> Maybe: "Record the current time and append new sample, removing the oldest 
Done
Line 319:         """
Line 320:         timestamp = self._timefn()
Line 321:         self._samples.append((timestamp, value))
Line 322: 


Line 322: 
Line 323:     def stats(self):
Line 324:         """
Line 325:         Return a tuple in the format: (first, last, difftime), containing
Line 326:         the first and the last return value in the defined 'window' and the
> BTW, I prefer the imperative tense, as that's what pep8 suggests.
Changed as Nir suggested, but using (everywhere) imperative tense as Dan prefers.
Line 327:         time difference.
Line 328:         """
Line 329:         if len(self._samples) < 2:
Line 330:             return None, None, None


-- 
To view, visit http://gerrit.ovirt.org/33783
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I48fe9e7f8ed3f5ec677d8f91a82cb70d0bdbb2e7
Gerrit-PatchSet: 4
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list