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