Change in vdsm[master]: clusterLock: Acquire, release, and inquire volume leases

nsoffer at redhat.com nsoffer at redhat.com
Sun Apr 12 15:53:12 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: clusterLock: Acquire, release, and inquire volume leases
......................................................................


Patch Set 8:

(2 comments)

Mostly ok, but if we add new api, we need to rename to old api so both of them make sense. Since this not public api, there is no reason to keep the old names.

I think we should have:

- acquireClusterLock
- releaseClusterLock
- inquireClusterLock

- acquireResource
- releaseResource
- inquireResource

What do you think?

https://gerrit.ovirt.org/#/c/38622/8/vdsm/storage/clusterlock.py
File vdsm/storage/clusterlock.py:

Line 297:         # timestamp, sanlock will return HOST_UNKNOWN and then HOST_LIVE or
Line 298:         # HOST_FAIL.
Line 299:         return HOST_STATUS_FREE
Line 300: 
Line 301:     def _acquire(self, resource, lockDisk, shared=False):
Please move _private method to the end of the class, or to the end of the section using the private method. It should be easy to understand what is the public interface of the class by reading the code from top to bottom.
Line 302:         with nested(self._lock, SANLock._sanlock_lock):
Line 303:             self.log.info("Acquiring resource %s, shared=%s", resource, shared)
Line 304: 
Line 305:             while True:


Line 337:                        "(id: %s)", self._sdUUID, hostId)
Line 338: 
Line 339:     def acquireResource(self, resource, lockDisk, shared=False):
Line 340:         self._acquire(resource, lockDisk, shared)
Line 341:         res, owners = self._inquire(resource, lockDisk)
Why do we need to call _inquire() here?

Add same logging as in acquire before and after the operation.
Line 342: 
Line 343:     def _inquire(self, resource, lockDisk):
Line 344:         res = sanlock.read_resource(*lockDisk[0])
Line 345:         owners = sanlock.read_resource_owners(self._sdUUID, resource, lockDisk)


-- 
To view, visit https://gerrit.ovirt.org/38622
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icca901ccd27358767c023cd55b7a3823531d2a5a
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list