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 <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
