Nir Soffer has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 6: Code-Review-1 (6 comments) The actual code looks very nice, but __eq__ is completely wrong. https://gerrit.ovirt.org/#/c/61435/6/lib/vdsm/storage/guarded.py File lib/vdsm/storage/guarded.py: Line 77: self._release() Line 78: except ReleaseError: Line 79: log.exception("Error releasing locks") Line 80: six.reraise(exc[0], exc[1], exc[2]) Line 81: self._held_locks.append(lock) __enter__ should return self, so you can use: with context as ctx: ... See https://docs.python.org/2/reference/datamodel.html#object.__enter__ Line 82: Line 83: def __exit__(self, exc_type, exc_val, exc_tb): Line 84: try: Line 85: self._release() Line 85: self._release() Line 86: except ReleaseError: Line 87: if exc_type is not None: Line 88: log.exception("Error releasing locks") Line 89: six.reraise(exc_type, exc_val, exc_tb) You don't need to and should not raise the original exception, it will be raised by Python if this method return falsy value. See https://docs.python.org/2/reference/datamodel.html#object.__exit__ What we need to do here is: - If the context was exited because of an error (exc_type is not None) - log release error, other wise we are hiding them. - if the context was exited normally, raise release error Line 90: raise Line 91: Line 92: def _release(self): Line 93: errors = [] Line 100: if errors: Line 101: raise ReleaseError(errors) Line 102: Line 103: Line 104: class LockInterface(object): This is not an interface but a abstract class, maybe call this AbstractLock? Line 105: @property Line 106: def ns(self): Line 107: raise NotImplementedError Line 108: Line 120: def release(self): Line 121: raise NotImplementedError Line 122: Line 123: def __repr__(self): Line 124: return repr(self._key()) I think we should use standard format: <ClassName ns=... name=... mode=... at 0x12345678> This is not a tuple, why should we show this a s tuple? Line 125: Line 126: def __eq__(self, other): Line 127: return self._key() == other._key() Line 128: Line 135: def __hash__(self): Line 136: return hash(self._key()) Line 137: Line 138: def _key(self): Line 139: return type(self), self.ns, self.name, self.mode This must be implemented by the subclasses, so __eq__ will return true only for equal locks. This code will treat locks with same sd_id but different domain or image as same locks, so we would actually acquire only one of them instead of all of them! Line 140: Line 141: Line 142: class ResourceManagerLock(LockInterface): Line 143: def __init__(self, ns, name, mode): Line 182: return self._vol_id Line 183: Line 184: @property Line 185: def mode(self): Line 186: return self.MODE Why not: return "exclusive" Or better use the constants defined in resourceManager (LockType.exclusive?) Line 187: Line 188: def acquire(self): Line 189: dom = sdCache.produce_manifest(self._sd_id) Line 190: dom.acquireVolumeLease(self._host_id, self._img_id, self._vol_id) -- 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: 6 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]
