Change in vdsm[master]: domainMonitor: Extract domain monitoring methods

nsoffer at redhat.com nsoffer at redhat.com
Sun Jun 1 20:29:59 UTC 2014


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 <nsoffer at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Xavi Francisco <xfrancis at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list