Nir Soffer has posted comments on this change. Change subject: sd: add inquireClusterLock method to StorageDomain ......................................................................
Patch Set 6: Code-Review-1 (3 comments) Looks good but - We must cleanup the repeating code for getting host id and lock and returning None, None when not set. - We should improve the name of the map keeping host ids and locks http://gerrit.ovirt.org/#/c/21426/6/vdsm/storage/clusterlock.py File vdsm/storage/clusterlock.py: Line 294: Line 295: STATIC_LVER = 0 Line 296: Line 297: _globalLockMap = {} Line 298: _globalLockMapSync = threading.Lock() Now this map keeps locks and host ids, so this name is not correct. Lets improve the names: _leases = {} _leasesLock = threading.Lock() global is not needed, and Map is redundant. These long names only makes the code harder to work with. Line 299: Line 300: def __init__(self, sdUUID, idsPath, leasesPath, *args): Line 301: self._sdUUID = sdUUID Line 302: self._idsPath = idsPath Line 321: Line 322: def acquireHostId(self, hostId, async): Line 323: with self._globalLockMapSync: Line 324: currentHostId, lockFile = self._globalLockMap.get( Line 325: self._sdUUID, (None, None)) We repeat this 6(!) times here. Lets add a method: def _getLease(self): return self._globalLockMap.get(self._sdUUID, (None, None)) Not sure that _getLease is the correct samantics, but _getHostIdAndLock seems too long and specific. I guess that Federico can find a better name. Line 326: Line 327: if currentHostId is not None and currentHostId != hostId: Line 328: self.log.error("Different host id already acquired (id: %s)", Line 329: currentHostId) Line 352: Line 353: self.log.debug("Host id for domain %s released successfully (id: %s)", Line 354: self._sdUUID, hostId) Line 355: Line 356: def hasHostId(self, hostId): Note for later: this should be containsHostId or includesHostId. Line 357: with self._globalLockMapSync: Line 358: currentHostId, lockFile = self._globalLockMap.get( Line 359: self._sdUUID, (None, None)) Line 360: return currentHostId == hostId -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
