Nir Soffer has posted comments on this change. Change subject: sp: improve domainStateChange event handling ......................................................................
Patch Set 3: (4 comments) https://gerrit.ovirt.org/#/c/51393/3//COMMIT_MSG Commit Message: Line 8: Line 9: onDomainStateChange is registered in the constructor of StoragePool. Line 10: This is a waste of resources since these events can be called only after Line 11: the connect method is called (specifically, after __rebuild is called Line 12: from within connect). > The main issue is that it is never unregistered, so each time you connect t The main issue (leaking registration) is non-issue, multiple registration replace the same registration and does not leak anything. Line 13: Line 14: This patch moves the registration to onDomainStateChange to be right Line 15: before it can actually be called (in connect), and calls unregister if Line 16: an error has occurred in __rebuild. https://gerrit.ovirt.org/#/c/51393/3/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 635 Line 636 Line 637 Line 638 Line 639 > This should be reverted if next steps fail, and raise if pool is already co We can wait with this for later patch, connect is called only if pool is not connected, under exclusive pool lock. Raising if pool is connect is good idea anyway but not required for this patch. Line 641 Line 642 Line 643 Line 644 Line 645 If this raises, the pool is not considered connected, and we should stop watching domain state changes. So the try expect should extended to contain also __createMailboxMonitor. Line 649: self.id = hostID Line 650: # Make sure SDCache doesn't have stale data (it can be in case of FC) Line 651: sdCache.invalidateStorage() Line 652: sdCache.refresh() Line 653: self._startWatchingDomainsState() > what will happen if we are conencted to the pool and issue connect call aga I checked misc.Event - multiple registrations with same function replace the function in the dict and are safe. We always register the same function - self._domainStageChangeCallback. Line 654: try: Line 655: # Rebuild whole Pool Line 656: self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion) Line 657: except Exception: -- To view, visit https://gerrit.ovirt.org/51393 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If116100d3ae967f6a5490a2d91bf923e953cb4ee Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Idan Shaby <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
