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