Federico Simoncelli has posted comments on this change. Change subject: domainMonitor: Extract domain monitoring methods ......................................................................
Patch Set 1: Code-Review-1 (6 comments) Requires some minor changes here and there. Marking with -1 for now. http://gerrit.ovirt.org/#/c/27714/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 191 Line 192 Line 193 Line 194 Line 195 > A function should be consistent in the level of abstraction it uses. So kee Third option put the check inside the same method and just have: self._refreshIfNeeded() Anyway I'm fine with all three solutions. Line 186: try: Line 187: self._initializeDomain() Line 188: self._checkDomain() Line 189: self._checkReadDelay() Line 190: self._collectStats() > _collectStatistics() is indeed nicer. +1 Line 191: except Exception as e: Line 192: self.log.error("Error while collecting domain %s monitoring " Line 193: "information", self.sdUUID, exc_info=True) Line 194: self.nextStatus.error = e Line 236: Line 237: def _initializeDomain(self): Line 238: # We should produce the domain inside the monitoring loop because Line 239: # it might take some time and we don't want to slow down the thread Line 240: # start (and anything else that relies on that as for example > I think you should read this comment more carefully. This is important comm +1 to keep the comment, it's important to remember why we need to check this every iteration. Anyway probably the comment belongs in the loop and not here. Line 241: # updateMonitoringThreads). It also needs to be inside the loop Line 242: # since it might fail and we want keep trying until we succeed or Line 243: # the domain is deactivated. Line 244: if self.domain is None: Line 242: # since it might fail and we want keep trying until we succeed or Line 243: # the domain is deactivated. Line 244: if self.domain is None: Line 245: self.domain = sdCache.produce(self.sdUUID) Line 246: > I agree, this function is doing now two unrelated things. I'll separate thi -2 to the suggestion to remove the comment. The proof that it must remain is that the code you suggested is exactly what we wanted to prevent. (BTW python is full of "is" methods) isIsoDomain should remain public. Even if it's not used (have you checked it?), it's the only way to know if a domain is iso without going to storage and eventually get stuck. For these immutable states we should start thinking of the domain monitor as an handy cache. Line 247: if self.isIsoDomain is None: Line 248: # The isIsoDomain assignment is delayed because the isoPrefix Line 249: # discovery might fail (if the domain suddenly disappears) and Line 250: # we could risk to never try to set it again. Line 285: def _shouldAcquireHostId(self): Line 286: # An ISO domain can be shared by multiple pools Line 287: return (not self.isIsoDomain and Line 288: self.nextStatus.valid and Line 289: self.nextStatus.hasHostId is False) > I don't think you need a PHD to understand this, but the way you suggest is The boolean wasn't that hard to understand but I like the documented version slightly better. Line 290: Line 291: def _shouldReleaseHostId(self): Line 292: # If this is an ISO domain we didn't acquire the host id and releasing Line 293: # it is superfluous. Line 289: self.nextStatus.hasHostId is False) Line 290: Line 291: def _shouldReleaseHostId(self): Line 292: # If this is an ISO domain we didn't acquire the host id and releasing Line 293: # it is superfluous. > I think that the current condition is very clear. The explaining variable y The only thing I am interested in here is that we fix this check. I have the impression that isIsoDomain could be None even if we pass the self.doman check. I think the check should be (here or in another patch, wherever you prefer): return self.isIsoDomain is False Or anything you deem clearer. Line 294: return self.domain and not self.isIsoDomain Line 295: Line 296: def _acquireHostId(self): Line 297: try: -- To view, visit http://gerrit.ovirt.org/27714 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I87ac6a82e560bc4360a3bc3f6f6fd94623678cc2 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Xavi Francisco <xfran...@redhat.com> Gerrit-Reviewer: Yoav Kleinberger <yklei...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
