Change in vdsm[master]: virt: add ExpiringDict
fromani at redhat.com
fromani at redhat.com
Mon Feb 2 13:16:02 UTC 2015
Francesco Romani has posted comments on this change.
Change subject: virt: add ExpiringDict
......................................................................
Patch Set 6:
(10 comments)
http://gerrit.ovirt.org/#/c/36716/6/vdsm/virt/utils.py
File vdsm/virt/utils.py:
Line 38: required = ('domainID', 'imageID', 'poolID', 'volumeID')
Line 39: return all(k in drive for k in required)
Line 40:
Line 41:
Line 42: class ExpiringDict(object):
> This name is not a good idea; users will expect this to behave like a dict,
Good point.
Line 43: """
Line 44: ExpiringDict behaves like a dict, but an expiration time
Line 45: is attached to each key. Thread safe.
Line 46:
Line 67: value = self._get(key)
Line 68: if value is not None:
Line 69: return value
Line 70: else:
Line 71: return default
> The if-else block should be out of the lock - there is no reason to hold th
Done - but locking inside _get() like you suggested below.
Line 72:
Line 73: def clear(self):
Line 74: self._items.clear()
Line 75:
Line 70: else:
Line 71: return default
Line 72:
Line 73: def clear(self):
Line 74: self._items.clear()
> Should be locked.
Done
Line 75:
Line 76: def __setitem__(self, key, value):
Line 77: with self._lock:
Line 78: self._items[key] = self._clock() + self._ttl, value
Line 74: self._items.clear()
Line 75:
Line 76: def __setitem__(self, key, value):
Line 77: with self._lock:
Line 78: self._items[key] = self._clock() + self._ttl, value
> Getting expiration time should be done out of the lock:
Done
Line 79:
Line 80: def __getitem__(self, key):
Line 81: with self._lock:
Line 82: value = self._get(key)
Line 82: value = self._get(key)
Line 83: if value is not None:
Line 84: return value
Line 85: else:
Line 86: raise KeyError(key)
> This is duplicate code of get().
Done
Line 87:
Line 88: def __delitem__(self, key):
Line 89: with self._lock:
Line 90: del self._items[key]
Line 88: def __delitem__(self, key):
Line 89: with self._lock:
Line 90: del self._items[key]
Line 91:
Line 92: def __contains__(self, key):
> We should not implement this. Checking if something is in an expiring cache
Makes sense. Done.
Line 93: with self._lock:
Line 94: return self._get(key) is not None
Line 95:
Line 96: def __len__(self):
Line 92: def __contains__(self, key):
Line 93: with self._lock:
Line 94: return self._get(key) is not None
Line 95:
Line 96: def __len__(self):
> We should not implement this for the same reason we should not implement __
Done
Line 97: # reminder: __len__ is used as fallback by __nonzero__
Line 98: with self._lock:
Line 99: now = self._clock()
Line 100: return sum(expiration > now
Line 104: # private
Line 105:
Line 106: def _get(self, key):
Line 107: """
Line 108: Return the None if invalid or expired key.
> the None -> None
Done
Line 109: Automatically cleanup expired keys.
Line 110: Caller must ensure proper locking.
Line 111: """
Line 112: if key not in self._items:
Line 109: Automatically cleanup expired keys.
Line 110: Caller must ensure proper locking.
Line 111: """
Line 112: if key not in self._items:
Line 113: return None
> Not needed - the next line will raise KeyError if the item does not exists.
Done
Line 114:
Line 115: expiration, value = self._items[key]
Line 116: if expiration <= self._clock():
Line 117: del self._items[key]
Line 111: """
Line 112: if key not in self._items:
Line 113: return None
Line 114:
Line 115: expiration, value = self._items[key]
> Take the lock here instead of get() and __getitem__ and other "read" method
Done
Line 116: if expiration <= self._clock():
Line 117: del self._items[key]
Line 118: return None
Line 119:
--
To view, visit http://gerrit.ovirt.org/36716
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I51e38cea6a23b7abe2375780e6ece0ff90831b6d
Gerrit-PatchSet: 6
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