Martin Polednik has posted comments on this change. Change subject: virt: support per-vm stats age ......................................................................
Patch Set 32: Code-Review-1 (4 comments) I wouldn't say anything is critical here, but some explanation about the pre/post sampling (in code or commit msg) would be nice for readers. https://gerrit.ovirt.org/#/c/38066/32/vdsm/virt/sampling.py File vdsm/virt/sampling.py: Line 424: Provide facilities to retrieve per-vm samples, Line 425: and the glue code to deal with disappearing per-vm samples. Line 426: """ Line 427: Line 428: EMPTY = StatsSample(None, None, None, None) # 'null' sample Should be in the same namespace as StatsSample imho. Line 429: Line 430: _log = logging.getLogger("sampling.StatsCache") Line 431: Line 432: def __init__(self, clock=utils.monotonic_time): Line 435: self._last_sample_time = 0 Line 436: self._vm_last_timestamp = defaultdict(int) Line 437: self._lock = threading.Lock() Line 438: Line 439: def clock(self): Making it a @property would be nicer from pythonic standpoint. Line 440: """ Line 441: Provide timestamp compatible with what put() expects Line 442: """ Line 443: return self._clock() Line 492: returned by unblocked stuck calls, to avoid overwrite fresh data Line 493: with stale one. Line 494: """ Line 495: with self._lock: Line 496: if monotonic_ts >= self._last_sample_time: Not exactly related to this patchset, but still something very close: is >= relation better idea than plain > here? Line 497: self._samples.append(bulk_stats) Line 498: self._last_sample_time = monotonic_ts Line 499: Line 500: self._update_ts(bulk_stats, monotonic_ts) Line 496: if monotonic_ts >= self._last_sample_time: Line 497: self._samples.append(bulk_stats) Line 498: self._last_sample_time = monotonic_ts Line 499: Line 500: self._update_ts(bulk_stats, monotonic_ts) Not sure which semantics did you target and I think it should be noted somewhere: Is it correct that the time of the last sample is calculated AFTER constructing the last sample? It could also be calculated BEFORE the sample is made where above comment would make sense. Line 501: else: Line 502: self._log.warning('dropped stale old sample') Line 503: Line 504: def _update_ts(self, bulk_stats, monotonic_ts): -- To view, visit https://gerrit.ovirt.org/38066 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id8cd75631c328aa380c10f1f6dd1a8075b2fe0ed Gerrit-PatchSet: 32 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: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@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
