Allon Mureinik has posted comments on this change. Change subject: core: adding support for db lock of all disk snapshots ......................................................................
Patch Set 2: (8 comments) Agree with all of Sergey's comments. Also, see inline. .................................................... Commit Message Line 6: Line 7: core: adding support for db lock of all disk snapshots Line 8: Line 9: This change is "part" of http://gerrit.ovirt.org/#/c/17679/ and is Line 10: separated just for review sake. Will this be merged separately or squashed into the backup API patch? Does it have any merrit on its own? Line 11: Line 12: This patch adds "support" for locking all disk snapshots/volumes as part of Line 13: the being added support for attaching disk snapshots to vms. Line 14: Line 9: This change is "part" of http://gerrit.ovirt.org/#/c/17679/ and is Line 10: separated just for review sake. Line 11: Line 12: This patch adds "support" for locking all disk snapshots/volumes as part of Line 13: the being added support for attaching disk snapshots to vms. This patch adds the ability to lock all disk snapshots/volumes as part of adding support for attaching disk snapshots to VMs. Line 14: Line 15: The added method in ImagesHandler can be used to lock all the disk Line 16: snapshots, while the current "lock" method locks the given volume only. Line 17: The need is for this change is to prevent race conditions, provide Line 13: the being added support for attaching disk snapshots to vms. Line 14: Line 15: The added method in ImagesHandler can be used to lock all the disk Line 16: snapshots, while the current "lock" method locks the given volume only. Line 17: The need is for this change is to prevent race conditions, provide /need is/need/ Line 18: ability to lock only the needed snapshots of a disk and to have correct Line 19: UI indication of the current state. Line 20: Line 21: Example use cases: Line 19: UI indication of the current state. Line 20: Line 21: Example use cases: Line 22: 1. When removing a disk, all the disk snapshots should be locked and Line 23: have its status reverted (otherwise they will be displayed as "OK" for s/its/their/ Line 24: vms that they are attached to). Line 25: 2. When extending a disk with snapshots, only the active snapshot should Line 26: be locked as other snapshots can be used as attached disk snapshots in Line 27: other vms. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java Line 646: Line 647: protected static void updateAllDiskImageSnapshotsStatusWithCompensation(final Guid diskId, Line 648: final ImageStatus status, Line 649: ImageStatus statusForCompensation, Line 650: final CompensationContext compensationContext) { Why do you need the final modifiers? They break convention with the over class methods. Line 651: Line 652: if (compensationContext != null) { Line 653: List<DiskImage> diskSnapshots = Line 654: DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForImageGroup(diskId); Line 648: final ImageStatus status, Line 649: ImageStatus statusForCompensation, Line 650: final CompensationContext compensationContext) { Line 651: Line 652: if (compensationContext != null) { I'd overload an additional updateAllDiskImageSnapshotsStatusWithCompensation(final Guid diskId, final ImageStatus status) method for this Line 653: List<DiskImage> diskSnapshots = Line 654: DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForImageGroup(diskId); Line 655: for (DiskImage diskSnapshot : diskSnapshots) { Line 656: diskSnapshot.setImageStatus(statusForCompensation); Line 650: final CompensationContext compensationContext) { Line 651: Line 652: if (compensationContext != null) { Line 653: List<DiskImage> diskSnapshots = Line 654: DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForImageGroup(diskId); Might as well use ImageDAO, since that's the only part you're touching. Line 655: for (DiskImage diskSnapshot : diskSnapshots) { Line 656: diskSnapshot.setImageStatus(statusForCompensation); Line 657: compensationContext.snapshotEntityStatus(diskSnapshot.getImage()); Line 658: } Line 654: DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForImageGroup(diskId); Line 655: for (DiskImage diskSnapshot : diskSnapshots) { Line 656: diskSnapshot.setImageStatus(statusForCompensation); Line 657: compensationContext.snapshotEntityStatus(diskSnapshot.getImage()); Line 658: } Also, shouldn't this block be performed inside the same transaction? Line 659: Line 660: TransactionSupport.executeInScope(TransactionScopeOption.Required, new TransactionMethod<Void>() { Line 661: @Override Line 662: public Void runInTransaction() { -- To view, visit http://gerrit.ovirt.org/20308 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibac8daf6a9e970859a6759d84d7849e7f84e0d79 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
