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