Nir Soffer has posted comments on this change. Change subject: domainMonitor: Extract domain monitoring methods ......................................................................
Patch Set 1: (6 comments) 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 > Third option put the check inside the same method and just have: Keeping the current solution. 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 > +1 to keep the comment, it's important to remember why we need to check thi I'll check moving this comment into the loop. 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: > -2 to the suggestion to remove the comment. The proof that it must remain i In another patch, we should convert isIsoDomain to read only property. 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 279: self.nextStatus.hasHostId = self.domain.hasHostId(self.hostId) Line 280: self.nextStatus.isoPrefix = self.isoPrefix Line 281: self.nextStatus.version = self.domain.getVersion() Line 282: Line 283: # Managing host id > Which is the primary function of namespaces... To organize code etc. Since nobody else complained about this yet, keeping this as is. Line 284: Line 285: def _shouldAcquireHostId(self): Line 286: # An ISO domain can be shared by multiple pools Line 287: return (not self.isIsoDomain and 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) > The boolean wasn't that hard to understand but I like the documented versio Will change in next patch. 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. > The only thing I am interested in here is that we fix this check. I have th In the next patch, I'll modify the code as Federico suggest. Here I like to have only separation of old code to new methods. Moving and modifying so much code is sure way to introduce bugs, and we have no tests for this code. 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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Xavi Francisco <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[email protected]> Gerrit-Reviewer: [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
