Allon Mureinik has posted comments on this change.
Change subject: core: WIP : RemoveImageDisk - race when updating snapshots ovf
(#828192)
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(5 inline comments)
Please consider the RemoveVM scenario - it's probably a good idea to have a way
to supress the ovf's upadate.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java
Line 103:
Line 104: private void removeImageFromDB() {
Line 105: final DiskImage diskImage = getDiskImage();
Line 106:
Line 107: updateSnapshotConfigWithoutImage(diskImage.getId());
The idea in separating the calculation of the OVF and it's update was to create
transactions as short as possible.
Here's you've separated it into two trnasactions, which is wrong - just move it
into the same transaction.
Line 108:
TransactionSupport.executeInScope(TransactionScopeOption.Required,
Line 109: new TransactionMethod<Object>() {
Line 110: @Override
Line 111: public Object runInTransaction() {
Line 190: private EngineLock snapshotEngineLock = new EngineLock();
Line 191: private Map<String, String> snapshotsExlusiveLockMap = new
HashMap<String, String>();
Line 192:
Line 193: private void lockSnapshotWithWait(Guid snapshotId) {
Line 194: snapshotsExlusiveLockMap.clear();
why keep a map?
why not just create it here?
Line 195: snapshotsExlusiveLockMap.put(snapshotId.toString(),
LockingGroup.SNAPSHOT.name());
Line 196:
snapshotEngineLock.setExclusiveLocks(snapshotsExlusiveLockMap);
Line 197: getLockManager().acquireLockWait(snapshotEngineLock);
Line 198: }
....................................................
Commit Message
Line 6:
Line 7: core: WIP : RemoveImageDisk - race when updating snapshots ovf (#828192)
Line 8:
Line 9: https://bugzilla.redhat.com/show_bug.cgi?id=828192
Line 10:
General: use an Upper Case letter after a period.
Line 11: when removing an image disk it should be removed from all the snapshots
Line 12: it's contained within. the removal from the image snapshots includes an
Line 13: update to the snapshot ovf (saved in the DB). the update is an
Line 14: read-update-write operation, so when two or more disks are removed from
Line 8:
Line 9: https://bugzilla.redhat.com/show_bug.cgi?id=828192
Line 10:
Line 11: when removing an image disk it should be removed from all the snapshots
Line 12: it's contained within. the removal from the image snapshots includes an
s/snapshots t's contained within/snapshots that contain it/
s/image snapshots/vm snapshots/
Line 13: update to the snapshot ovf (saved in the DB). the update is an
Line 14: read-update-write operation, so when two or more disks are removed from
Line 15: the same snapshot a race condition might occur.
Line 16: this patch adds a lock on the snapshot when performing the operations
on
Line 9: https://bugzilla.redhat.com/show_bug.cgi?id=828192
Line 10:
Line 11: when removing an image disk it should be removed from all the snapshots
Line 12: it's contained within. the removal from the image snapshots includes an
Line 13: update to the snapshot ovf (saved in the DB). the update is an
s/an/a/
Line 14: read-update-write operation, so when two or more disks are removed from
Line 15: the same snapshot a race condition might occur.
Line 16: this patch adds a lock on the snapshot when performing the operations
on
Line 17: the snapshot ovf to prevent the race condition.
--
To view, visit http://gerrit.ovirt.org/7482
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccb44f1aa9d204477955343167133849a4146753
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
