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

ykleinbe at redhat.com ykleinbe at redhat.com
Sun May 18 19:24:04 UTC 2014


Yoav Kleinberger has posted comments on this change.

Change subject: domainMonitor: Extract domain monitoring methods
......................................................................


Patch Set 1: Code-Review+1

(14 comments)

great work! I left some notes on points I think can be further improved

http://gerrit.ovirt.org/#/c/27714/1/vdsm/storage/domainMonitor.py
File vdsm/storage/domainMonitor.py:

Line 180
Line 181
Line 182
Line 183
Line 184
nice, but see below


Line 191
Line 192
Line 193
Line 194
Line 195
the _shouldRefreshDomain() is not needed here. I think 

if time() - self.lastRefresh > self.refreshTime:
   self._refreshDomain()

is enough improvement. The conditional is clear.


Line 186:         try:
Line 187:             self._initializeDomain()
Line 188:             self._checkDomain()
Line 189:             self._checkReadDelay()
Line 190:             self._collectStats()
I hate shorthand. _collectStatistics, not _collectStats
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 225:     def _shouldRefreshDomain(self):
Line 226:         return time() - self.lastRefresh > self.refreshTime
Line 227: 
Line 228:     def _refreshDomain(self):
Line 229:         # Refreshing the domain object in order to pick up changes as,
since you have now surrounded the code with a function with a descriptive name, why not go ahead and DELETE THE ANNOYING COMMENTS which are totally unneeded.
Line 230:         # for example, the domain upgrade.
Line 231:         self.log.debug("Refreshing domain %s", self.sdUUID)
Line 232:         sdCache.manuallyRemoveDomain(self.sdUUID)
Line 233:         self.lastRefresh = time()


Line 234: 
Line 235:     # Deferred initialization
Line 236: 
Line 237:     def _initializeDomain(self):
Line 238:         # We should produce the domain inside the monitoring loop because
looks like this function is not exactly and initializer.
rather, it "obtains" or "acquires" something in case it's not yet here. Consider a different name.
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
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 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
refactoring is good, but leaving old outdated comments is bad. Now the comments are inside this function, which knows nothing of the "loop" to which the comment refers. 

destroy the comments - the comments were there in the first place because the code was unreadable. Now it is, so we don't need them anymore.
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: 
the whole isIsoDomain stuff should be extracted to a different function, with a descriptive name that will allow us to delete these comments. Also, a name with "is" at the beginnig is evil. Oh, and shouldn't it be private?

def _ensureIsoDomainAssignement( self ): # think of some better name
   if self._isoDomain is not None:
      return
   self._isoDomain = self.domain.isISO()
   if self._isoDomain:
      self._isoPrevix = self.domain.getIsoDomainImagesDir()
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 255: 
Line 256:     # Collecting monitoring info
Line 257: 
Line 258:     def _checkDomain(self):
Line 259:         self.domain.selftest()
maybe rename to _checkDomainIntegrity ?
Line 260: 
Line 261:     def _checkReadDelay(self):
Line 262:         self.nextStatus.readDelay = self.domain.getReadDelay()
Line 263: 


Line 258:     def _checkDomain(self):
Line 259:         self.domain.selftest()
Line 260: 
Line 261:     def _checkReadDelay(self):
Line 262:         self.nextStatus.readDelay = self.domain.getReadDelay()
is this a "check"? looks more like a "set" to me. 
Either give it a more descriptive name that interprets the line it wraps, or revert to the old form.
Line 263: 
Line 264:     def _collectStats(self):
Line 265:         stats = self.domain.getStats()
Line 266:         self.nextStatus.diskUtilization = (stats["disktotal"],


Line 261:     def _checkReadDelay(self):
Line 262:         self.nextStatus.readDelay = self.domain.getReadDelay()
Line 263: 
Line 264:     def _collectStats(self):
Line 265:         stats = self.domain.getStats()
the word "stats" is in the function's name. So we can use "domainInfo" and "masterInfo" inside and the code will be less annoying to read. It is clear from the context that the info is in fact, statistics.
Line 266:         self.nextStatus.diskUtilization = (stats["disktotal"],
Line 267:                                            stats["diskfree"])
Line 268: 
Line 269:         self.nextStatus.vgMdUtilization = (stats["mdasize"],


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
delete this comment. the function names should be clear enough
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)
not clear enough for someone who doesn't know more about storage than I do.

do we *really* need this complex boolean expression? it's hard for a human to figure out.

if self._isoDomain:
  return False
if not self.nextStatus.valid:
  return False
return not self.nextStatus.hasHostId
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.
maybe I'm wrong, but looks like self.domain is a separate condition, so first do

if not self.domain:
  return False

then, delete the annoying comments. Instead, make the code more readable like this:

acquiredPreviously = not self.isIsoDomain
return acquiredPreviously
Line 294:         return self.domain and not self.isIsoDomain
Line 295: 
Line 296:     def _acquireHostId(self):
Line 297:         try:


Line 305:         try:
Line 306:             self.domain.releaseHostId(self.hostId, unused=True)
Line 307:         except:
Line 308:             self.log.debug("Unable to release the host id %s for domain "
Line 309:                            "%s", self.hostId, self.sdUUID, exc_info=True)
this pattern of try/except/logging can be refactored into a nice decorator - which will clean up the code a bit.


-- 
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: 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