Nir Soffer has posted comments on this change. Change subject: sd: add inquireClusterLock method to StorageDomain ......................................................................
Patch Set 5: Code-Review-1 (4 comments) This patch has +1 from me and +2 from Ayal - why did you change it? http://gerrit.ovirt.org/#/c/21426/5/vdsm/storage/clusterlock.py File vdsm/storage/clusterlock.py: Line 41: SDM_LEASE_NAME = 'SDM' Line 42: SDM_LEASE_OFFSET = 512 * 2048 Line 43: Line 44: Line 45: class InquireNotImplementedError(RuntimeError): - I would replace this with NotSupportedError, and move it to storage_exception.py, so we can reuse the concept of something which does not support some operation. Not implemented should not be part of an api, usually this means programmer error. This will lead to bugs asking to implement this feature. - Using or inheriting from RuntimeError is a bad idea - we don't want to mix our error with some random error in the standard library. This will lead pointless exception showing our InquireNotImplementedError, when code try to catch RuntimeError. We already have places that do this, but I would like not to have new instances. Line 46: pass Line 47: Line 48: Line 49: class SafeLease(object): Line 42: SDM_LEASE_OFFSET = 512 * 2048 Line 43: Line 44: Line 45: class InquireNotImplementedError(RuntimeError): Line 46: pass Having a docstring that explain the error is more useful then "pass": class FooError(Exception): """ Raised when ... """ Line 47: Line 48: Line 49: class SafeLease(object): Line 50: log = logging.getLogger("SafeLease") Line 318: Line 319: def getReservedId(self): Line 320: return MAX_HOST_ID Line 321: Line 322: def acquireHostId(self, hostId, async): This is not related to this patch - please keep this patch focused about the new api. Line 323: with self._globalLockMapSync: Line 324: currentHostId, lockFile = self._globalLockMap.get( Line 325: self._sdUUID, (None, None)) Line 326: http://gerrit.ovirt.org/#/c/21426/5/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 657: self.log.error("Create pool %s canceled ", poolName, exc_info=True) Line 658: try: Line 659: fileUtils.cleanupdir(self.poolPath) Line 660: self.__cleanupDomains(domList, msdUUID, masterVersion) Line 661: except: except:?! why use bare except here? Line 662: self.log.error("Cleanup failed due to an unexpected error", Line 663: exc_info=True) Line 664: raise Line 665: finally: -- To view, visit http://gerrit.ovirt.org/21426 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10e64d74319ea6591a7edf8e17809d367a758386 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
