Change in vdsm[master]: BZ#744704 - Add rollback support for complex tasks

danken at redhat.com danken at redhat.com
Wed Oct 19 10:29:13 UTC 2011


Dan Kenigsberg has posted comments on this change.

Change subject: BZ#744704 - Add rollback support for complex tasks
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(5 inline comments)

....................................................
File vdsm/storage/blockVolume.py
Line 249:             cls.log.warn("Cannot deactivate new created volume %s/%s", sdUUID, volUUID, exc_info=True)
Line 250: 
Line 251:         # Remove all previous rollbacks for 'halfbaked' volume and add rollback for 'real' volume creation
Line 252:         vars.task.replaceRecoveries(task.Recovery("create block volume rollback", "blockVolume", "BlockVolume", "createVolumeRollback",
Line 253:                                              [repoPath, sdUUID, imgUUID, volUUID, imageDir]), True)
seems like a good place to use python's named args..
I almost forgot what True means here.
Line 254:         return volUUID
Line 255: 
Line 256: 
Line 257:     def delete(self, postZero, force):


....................................................
File vdsm/storage/task.py
Line 73: FIELD_SEP = ","
Line 74: RESOURCE_SEP = "!"
Line 75: TASK_METADATA_VERSION = 1
Line 76: 
Line 77: STUB_ROLLBACK = "stub rollback"
that's actually commony called "sentinel"
http://en.wikipedia.org/wiki/Sentinel_value
Line 78: 
Line 79: class State:
Line 80:     unknown = "unknown"
Line 81:     init = "init"


Line 1047:         self.recoveries.append(recovery)
Line 1048:         self.persist()
Line 1049: 
Line 1050: 
Line 1051:     def replaceRecoveries(self, recovery, singleTask=False):
Is there ANY need to keep the former semantic? Removing unrelated tasks feels wrong and misleading. Who needs it? Have him call another replaceAllRecoveries function.
Line 1052:         if not isinstance(recovery, Recovery):
Line 1053:             raise TypeError("recovery param %s(%s) must be Recovery object" % (repr(recovery), type(recovery)))
Line 1054:         if not recovery.name:
Line 1055:             raise ValueError("replaceRecoveries: name is required")


....................................................
File vdsm/storage/volume.py
Line 327:     @classmethod
Line 328:     def startCreateVolumeRollback(cls, taskObj, sdUUID, imgUUID, volUUID):
Line 329:         cls.log.info("startCreateVolumeRollback: sdUUID=%s imgUUID=%s "\
Line 330:                         "volUUID=%s " % ( sdUUID, imgUUID, volUUID))
Line 331:         # This rollback it's actually do nothing.
it's --> doesn't
Line 332:         # In general the createVolume rollbacks are a list of small rollbacks
Line 333:         # that replaced by the one major rollback at the end of the task.
Line 334:         # This rollback is a simple marker that must to be the first rollback
Line 335:         # in the list of createVolume rollbacks.


Line 329:         cls.log.info("startCreateVolumeRollback: sdUUID=%s imgUUID=%s "\
Line 330:                         "volUUID=%s " % ( sdUUID, imgUUID, volUUID))
Line 331:         # This rollback it's actually do nothing.
Line 332:         # In general the createVolume rollbacks are a list of small rollbacks
Line 333:         # that replaced by the one major rollback at the end of the task.
that replaced -> that are replaced
Line 334:         # This rollback is a simple marker that must to be the first rollback
Line 335:         # in the list of createVolume rollbacks.
Line 336:         # We need it in cases when createVolume is a part of more complex task and
Line 337:         # not a task by itself. In such cases when we will replace the list of


--
To view, visit http://gerrit.usersys.redhat.com/1041
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie36ac02475f3f6438a63fbcbd26730862216cebd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Ayal Baron
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list