Nir Soffer has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 1: (8 comments) Nice! I'll continue tomorrow. https://gerrit.ovirt.org/#/c/61435/1/lib/vdsm/storage/guarded.py File lib/vdsm/storage/guarded.py: Line 26: log = logging.getLogger('storage.guarded') Line 27: rmanager = rm.ResourceManager.getInstance() Line 28: Line 29: Line 30: class operation_context(object): This name is not clear - why operation? How about naming the module name to guard.py and calling this context? from vdsm.storage import guard ... with guard.context(src, dst): ... Or maybe: from vdsm.storage import locking ... with locking.context(src, dst): ... Line 31: Line 32: def __init__(self, entities): Line 33: self._rm_locks = self._collect_rm_locks(entities) Line 34: self._held_locks = [] Line 49: locks = [] Line 50: for entity in entities: Line 51: locks.extend(entity.get_rm_locks()) Line 52: Line 53: # Exclusive locks always take precedence over shared This is not very clear - do you mean that if we get both shared and exclusive lock on same ns and name, we would take only the exclusive lock? Are you sure this is not a client error to request such combination? maybe we should raise in this case? Line 54: lock_set = set([l for l in locks if l.mode == rm.LockType.exclusive]) Line 55: shared_locks = set([l for l in locks if l.mode == rm.LockType.shared]) Line 56: lock_set.update(shared_locks) Line 57: return sorted(lock_set) Line 58: Line 59: def _release(self): Line 60: while self._held_locks: Line 61: lock = self._held_locks.pop() Line 62: lock.release() We need to catch and log exceptions here, and continue releasing. Line 63: Line 64: Line 65: class Entity(object): Line 66: """ Line 67: Defines the interface that must be implemented by objects that wish to be Line 68: guarded by operation_context. Line 69: """ Line 70: Line 71: def get_rm_locks(self): Can we avoid get_ prefix? Maybe call this "resources" or "locks"? When we add ClusterLock or CluserLease, using same interface (acquire, release), we will not need to use resource manager locks and cluster locks differently. We just need to make cluster locks sort in the right place using some namespace (00_cluser, 01_local, 02_locak_activation, ...)? Line 72: return [] Line 73: Line 74: Line 75: class ResourceManagerLock(object): Line 71: def get_rm_locks(self): Line 72: return [] Line 73: Line 74: Line 75: class ResourceManagerLock(object): Nice! Line 76: def __init__(self, ns, name, mode): Line 77: self.ns = ns Line 78: self.name = name Line 79: self.mode = mode Line 84: def release(self): Line 85: rmanager.releaseResource(self.ns, self.name) Line 86: Line 87: def _key(self): Line 88: return self.ns, self.name We don't really need this Line 89: Line 90: def __cmp__(self, other): Line 91: return cmp(self._key(), other._key()) Line 92: Line 87: def _key(self): Line 88: return self.ns, self.name Line 89: Line 90: def __cmp__(self, other): Line 91: return cmp(self._key(), other._key()) Do you want to put these objects into a set for removing duplicate locks? This is not compatible with python 3, you should implement __lt__ and __eq__ instead, see vdsm.schedule.ScheduledCall and vdsm.storage.asynchat.Timer for example. def __lt__(self, other): return (self.ns, self.name) < (other.ns, other.name) def __eq__(self, other): return (type(self) is type(other) and self.ns == other.ns and self.name == other.name and self.mode == other.mode) Line 92: Line 93: def __hash__(self): Line 90: def __cmp__(self, other): Line 91: return cmp(self._key(), other._key()) Line 92: Line 93: def __hash__(self): Line 94: return hash(self._key()) Then this hash is wrong, ignoring both the class and the mode. It should be: return hash((self.__class__, self.ns, self.name, self.mode)) -- To view, visit https://gerrit.ovirt.org/61435 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
