Nir Soffer has posted comments on this change.
Change subject: sampling.ImagePathStatus: drop unused code, and only it ......................................................................
Patch Set 1:
(4 comments)
Looks ok, can be cleaned up a bit.
.................................................... File vdsm/sampling.py Line 363: """ Line 364: AVERAGING_WINDOW = 5 Line 365: SAMPLE_INTERVAL_SEC = 2 Line 366: Line 367: def __init__(self, cif, log): I would accept irs instead of cif, since we do not use cif here - why depend on that class having an irs where we can get that irs directly?
Do we expect that cif.irs can become None while the application is running? Line 368: self.startTime = time.time() Line 369: Line 370: threading.Thread.__init__(self) Line 371: self._log = log
Line 416: def get(self): Line 417: stats = self._getInterfacesStats() Line 418: stats['cpuSysVdsmd'] = stats['cpuUserVdsmd'] = 0.0 Line 419: stats['storageDomains'] = self._getStorageDomainsStats() Line 420: Is this extra line intended? Line 421: now = time.time() Line 422: stats['elapsedTime'] = int(now - self.startTime) Line 423: if len(self._samples) < 2: Line 424: return stats
Line 417: stats = self._getInterfacesStats() Line 418: stats['cpuSysVdsmd'] = stats['cpuUserVdsmd'] = 0.0 Line 419: stats['storageDomains'] = self._getStorageDomainsStats() Line 420: Line 421: now = time.time() This temporary variable is pointless - I would inline it into line 422:
stats['elapsedTime'] = int(time.time() - self.startTime)
I think that this variable lead to the strange code in the previous version, where it seems like the code is trying to time the iteration over the dictionary, while it is not. Line 422: stats['elapsedTime'] = int(now - self.startTime) Line 423: if len(self._samples) < 2: Line 424: return stats Line 425: hs0, hs1 = self._samples[0], self._samples[-1]
Line 512: Line 513: return stats Line 514: Line 515: def _getStorageDomainsStats(self): Line 516: sds = {} This name "sds" is little confusing together with "sd" and "d". i would rename it to "stats" like in _getInterfacesStats(). The fact that this is "storage domain stats" is not important since there are no other types of stats in this scope. Line 517: if self._cif.irs: Line 518: res = self._cif.irs.repoStats() Line 519: del res["status"] Line 520: if "args" in res: