Change in vdsm[master]: virt: add ExpiringSet
nsoffer at redhat.com
nsoffer at redhat.com
Thu Jan 22 20:57:38 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: virt: add ExpiringSet
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.ovirt.org/#/c/36716/4//COMMIT_MSG
Commit Message:
Line 7: virt: add ExpiringSet
Line 8:
Line 9: an ExpiringSet is a plain set with an expiration time attached
Line 10: for each key.
Line 11: Will be used by future patches to handle gracefully timeouts.
It looks like you are trying to create a simple cache which items expire after some time.
Why not something simpler that work like a dict?
cache = Cache(maxage=10)
cache["key"] = value
"key" in cache -> true
time.sleep(10)
"key" in cache -> False
cache["key"] -> raise KeyError
This is more generic and can be used in many other places.
Line 12:
Line 13: Change-Id: I51e38cea6a23b7abe2375780e6ece0ff90831b6d
http://gerrit.ovirt.org/#/c/36716/4/vdsm/virt/utils.py
File vdsm/virt/utils.py:
Line 56: def remove(self, item):
Line 57: with self._lock:
Line 58: self._items.pop(item, None)
Line 59:
Line 60: def contains(self, item, timestamp=None):
This API is very strange. What is the meaning of testing if the set contains a value with a timestamp?
How are you going to use this?
Line 61: with self._lock:
Line 62: expiration = self._items.get(item)
Line 63: if expiration is None:
Line 64: return False
Line 63: if expiration is None:
Line 64: return False
Line 65: now = (
Line 66: self._timefn() if timestamp is None
Line 67: else timestamp)
This is less clear than a simple if:
if timestamp is None:
...
else:
...
Line 68: if expiration <= now:
Line 69: del self._items[item]
Line 70: return False
Line 71: return True
Line 73: def clear(self):
Line 74: self._items.clear()
Line 75:
Line 76: def __contains__(self, item):
Line 77: return self.contains(item)
Having both contains() and __contains__() is strange and confusing.
Line 78:
Line 79: def __len__(self):
Line 80: with self._lock:
Line 81: return len(self._items)
--
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: 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