Adam Litke has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 2: (10 comments) https://gerrit.ovirt.org/#/c/61435/2//COMMIT_MSG Commit Message: Line 23: Line 24: The following rules are currently enforced: Line 25: - Locks are collected from all entities and sorted Line 26: - Duplicate locks are removed Line 27: - Exclusive mode takes precedence over shared mode > We removed this feature, right? Yes, will update. Line 28: Line 29: Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 https://gerrit.ovirt.org/#/c/61435/2/lib/vdsm/storage/guarded.py File lib/vdsm/storage/guarded.py: Line 34: Provides a guarded context associated with the given entities. Each Line 35: entity must provide a locks property which holds an iterable of Line 36: guarded.Lock objects. The locks associated with the entities are Line 37: filtered for duplicates and sorted according to vdsm locking rules. Line 38: """ > We need a class docstring to explain about this class, here we describe the Done Line 39: self._locks = self._collect_locks(entities) Line 40: self._held_locks = [] Line 41: Line 42: def __enter__(self): Line 44: for lock in self._locks: Line 45: lock.acquire() Line 46: self._held_locks.append(lock) Line 47: except: Line 48: self._release() > Here we always want to log release failures, so: Done Line 49: raise Line 50: Line 51: def __exit__(self, exc_type, exc_val, exc_tb): Line 52: self._release() Line 47: except: Line 48: self._release() Line 49: raise Line 50: Line 51: def __exit__(self, exc_type, exc_val, exc_tb): > If releasing some locks failed, and there was no exception in the context ( Done Line 52: self._release() Line 53: Line 54: def _collect_locks(self, entities): Line 55: locks = set() Line 62: lock = self._held_locks.pop() Line 63: try: Line 64: lock.release() Line 65: except: Line 66: log.exception("Exception while releasing lock %r", lock) > One issue, we may like to report release failure if called from __exit__. Done Line 67: Line 68: Line 69: class Lock(object): Line 70: def __init__(self, ns, name, mode): Line 65: except: Line 66: log.exception("Exception while releasing lock %r", lock) Line 67: Line 68: Line 69: class Lock(object): > Lets document the purpose of this class, adding uniform interface to differ good idea. Will do. Line 70: def __init__(self, ns, name, mode): Line 71: self.ns = ns Line 72: self.name = name Line 73: self.mode = mode Line 69: class Lock(object): Line 70: def __init__(self, ns, name, mode): Line 71: self.ns = ns Line 72: self.name = name Line 73: self.mode = mode > Having __init__ in the base class will not work well for other lock types. ok. I'll add in VolumeLease next. Line 74: Line 75: def acquire(self): Line 76: raise NotImplementedError Line 77: Line 87: def __lt__(self, other): Line 88: return (self.ns, self.name) < (other.ns, other.name) Line 89: Line 90: def __hash__(self): Line 91: return hash(self._key()) > Hash should include also the type. Done Line 92: Line 93: def _key(self): Line 94: return self.ns, self.name, self.mode Line 95: Line 90: def __hash__(self): Line 91: return hash(self._key()) Line 92: Line 93: def _key(self): Line 94: return self.ns, self.name, self.mode > We can fix __hash__ and simplify __eq__ like this: Done Line 95: Line 96: Line 97: class ResourceManagerLock(Lock): Line 98: def __init__(self, ns, name, mode): Line 95: Line 96: Line 97: class ResourceManagerLock(Lock): Line 98: def __init__(self, ns, name, mode): Line 99: super(ResourceManagerLock, self).__init__(ns, name, mode) > We do nothing here, so we don't need __init__. Done Line 100: Line 101: def acquire(self): Line 102: rmanager.acquireResource(self.ns, self.name, self.mode) Line 103: -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> 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]
