Change in vdsm[master]: sd: add inquireClusterLock method to StorageDomain

nsoffer at redhat.com nsoffer at redhat.com
Wed Jan 15 14:12:22 UTC 2014


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 <fsimonce at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list