Nir Soffer has posted comments on this change. Change subject: sp: refactor out the metadata access from StoragePool ......................................................................
Patch Set 2: (4 comments) This patch should move some methods to the backend, keeping the pool api without change, except private methods. But is looks like this patch do change pool public api. If some pool apis are public now, but should be private, the change should be separate from the backend patch. http://gerrit.ovirt.org/#/c/22132/2/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 600: @staticmethod Line 601: def _getSpmStatusInfo(pool): Line 602: return dict( Line 603: zip(('spmStatus', 'spmLver', 'spmId'), Line 604: (pool.spmRole,) + pool.getBackend().getSpmStatus())) > why is getSpmStatus not wrapped by the pool? (i.e. instead of calling getBa +1 for hiding the backend behind the pool interface. Line 605: Line 606: @public Line 607: def getSpmStatus(self, spUUID, options=None): Line 608: pool = self.getPool(spUUID) Line 1035: self._updateStoragePool(pool, hostID, msdUUID, masterVersion) Line 1036: return True Line 1037: Line 1038: pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng) Line 1039: pool.backend = spbackends.StoragePoolMetaBackend(pool) > why is this not set in __init__? (see same comment in next patch since once We have circular reference here, which makes it hard to create these objects. It would be nice if the public object (pool), is created with the internal object (backend), and in pool.__init__ we can attach them togehter using: self.backend.pool = self Line 1040: Line 1041: # Must register domain state change callbacks *before* connecting Line 1042: # the pool, which starts domain monitor threads. Otherwise we will Line 1043: # miss the first event from the monitor thread. Line 1821: sd.validateSDStatus(status) Line 1822: except: Line 1823: domDict[d] = sd.validateSDDeprecatedStatus(status) Line 1824: Line 1825: return pool.getBackend().reconstructMaster( > again, expose reconstructMaster in pool object? +1 Line 1826: hostId, poolName, masterDom, domDict, masterVersion, leaseParams) Line 1827: Line 1828: def _logResp_getDeviceList(self, response): Line 1829: logableDevs = deepcopy(response) http://gerrit.ovirt.org/#/c/22132/2/vdsm/storage/spbackends.py File vdsm/storage/spbackends.py: Line 74: Line 75: log = logging.getLogger('Storage.StoragePoolMetaBackend') Line 76: Line 77: def __init__(self, pool): Line 78: self.pool = weakref.proxy(pool) If we set the pool after the object is created, we remove the dependency on the pool and make object creation easier. We can set the pool by: @property def pool(self): return self._pool @pool.setter def pooll(sefl, value): self._pool = weakref.proxy(value) Line 79: Line 80: ### Read-Only StoragePool Object Accessors ### Line 81: Line 82: def isSafe(self): -- To view, visit http://gerrit.ovirt.org/22132 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75493d1db60e51cccd5231b516f963c970d24c99 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@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
