Nir Soffer has posted comments on this change. Change subject: sp: refresh metadata on hsm when listing domains ......................................................................
Patch Set 4: Code-Review-1 (7 comments) There are 3 changes in this patch: - Encapsulating access to meta data using new setDomainsMap and getDomainsMap - good change - Invalidate meta data in getDomainsMap instead of in updateMonitoringThreads - looks very wrong to me. - Removal of the @unsecured decorators from some methods - I don't understand what this change does I suggest to split the refactoring from the behavior changes: 1. add getDomainsMap and setDomainsMap - can be merged quickly 2. Invalidation changes - if they are correct 3. Unsecured changes .................................................... Commit Message Line 3: AuthorDate: 2013-11-27 06:32:14 -0500 Line 4: Commit: Federico Simoncelli <[email protected]> Line 5: CommitDate: 2013-12-06 12:10:20 -0500 Line 6: Line 7: sp: refresh metadata on hsm when listing domains Please explain why we need to do this, and what happen if we don't. Line 8: Line 9: In this patch: Line 10: - validatePoolSD and validateAttachedDomain are relevant only for SPM Line 11: operations (remove @unsecured) Line 7: sp: refresh metadata on hsm when listing domains Line 8: Line 9: In this patch: Line 10: - validatePoolSD and validateAttachedDomain are relevant only for SPM Line 11: operations (remove @unsecured) I don't have any idea what is the meaning of @unsecured, but it is clear that this change is not related to this patch. Line 12: Line 13: Change-Id: I095cd0760076fb4be97a776498af78a40ff84112 .................................................... File vdsm/storage/sp.py Line 528 Line 529 Line 530 Line 531 Line 532 Please split this change to another patch. Line 536 Line 537 Line 538 Line 539 Line 540 Please split this change to another patch. Line 1540 Line 1541 Line 1542 Line 1543 Line 1544 Why did is not needed here now? Line 160: self.setMetaParams(metaParams, __securityOverride=True) Line 161: Line 162: @unsecured Line 163: def getDomainsMap(self): Line 164: self.invalidateMetadata() This looks very wrong - an accesor should not have side effects like this. Line 165: return self.getMetaParam(PMDK_DOMAINS) Line 166: Line 167: def setDomainsMap(self, domains): Line 168: self.setMetaParam(PMDK_DOMAINS, domains) Line 536: poolMeta[PMDK_MASTER_VER] = masterVersion Line 537: domain.changeRole(role) Line 538: Line 539: # TODO: Remove or rename this function. Line 540: def validatePoolSD(self, sdUUID): Now meta data is invalidated when calling self.getDomains(). Previously it was invalidated only if sdUUID was not in the returned list. Line 541: if sdUUID not in self.getDomains(): Line 542: raise se.StorageDomainNotMemberOfPool(self.spUUID, sdUUID) Line 543: return True Line 544: -- To view, visit http://gerrit.ovirt.org/21786 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I095cd0760076fb4be97a776498af78a40ff84112 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[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
