Francesco Romani has posted comments on this change. Change subject: virt: add ExpiringSet ......................................................................
Patch Set 4: (5 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 af This is a very good suggestion. I found myself recently in need of an utility like this, so I'll change in this direction. Line 12: Line 13: Change-Id: I51e38cea6a23b7abe2375780e6ece0ff90831b6d http://gerrit.ovirt.org/#/c/36716/4/tests/vmUtilsTests.py File tests/vmUtilsTests.py: Line 26: Line 27: class TestExpiringSet(TestCaseBase): Line 28: def setUp(self): Line 29: self._counter = itertools.count(0) Line 30: self.es = utils.ExpiringSet( > es is a hard to read name. Isn't this a cache? It is and will change name to "cache" Line 31: maxage=10, Line 32: timefn=lambda: next(self._counter)) Line 33: Line 34: def test_empty(self): 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 contain I started only with plain __contains__. But every __contains__ was calling timefn (time.time or utils.monotonic_time), and I was looking for an API which do not _requires_ such a barrage of system calls, for performance reasons. However, the resulting API is confusing and the benefits of recycling the timestamp are not obvious, so I'm going to drop it and have only __contains__. 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: Will change. 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. will remove contains(), will leave only __contains__ 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 <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
