Adam Litke has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 1: (7 comments) 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? I'll rename to guard.context. 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 exclusi Exactly. I'll reword it. I think there are valid scenarios when you'd have both types. For example, copying within an image, the source would specify a shared lock and the destination might want exclusive. 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. ok 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? Good idea. I can add a new NS and make it a class attribute for these locks. Line 72: return [] Line 73: Line 74: Line 75: class ResourceManagerLock(object): 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 see below. 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? I need these functions for two reasons: * __cmp__ to sort locks * __hash__ for putting into sets According to the docs, sets depend on hashable objects, not __lt__ and __eq__. 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. I want two locks that only differ in mode to go into the same hash bucket in order to eliminate duplicates. -- 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: 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]
