Maor Lipchuk has posted comments on this change. Change subject: core: adding support for db lock of all disk snapshots ......................................................................
Patch Set 3: (4 comments) Have you tested all the snapshot scenrios, we might probably need to re-think how to validate all the locks on the snapshot operations. .................................................... Commit Message Line 5: CommitDate: 2013-10-21 17:28:52 +0300 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 1. why "part" with quotes? 2. s/and is/and it's 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. Line 8: 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 why "support" with quotes? 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. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java Line 648: final ImageStatus status, Line 649: ImageStatus statusForCompensation, Line 650: final CompensationContext compensationContext) { Line 651: Line 652: if (compensationContext != null) { * The method name is misleading you are calling update...WithCompensation but sometime you pass null in the compensation parameters. * I think it would be better to use overload here for compensation Context or have two different methods. the if here is redundant and prone error if the user does not know what he is passing. Line 653: List<DiskImage> diskSnapshots = Line 654: DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForImageGroup(diskId); Line 655: for (DiskImage diskSnapshot : diskSnapshots) { Line 656: diskSnapshot.setImageStatus(statusForCompensation); .................................................... File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java Line 114: Image image = dao.get(EXISTING_IMAGE_ID); Line 115: List<DiskImage> snapshots = dbFacade.getDiskImageDao().getAllSnapshotsForImageGroup(image.getDiskId()); Line 116: assertFalse(snapshots.size() == 1); Line 117: for (DiskImage diskImage : snapshots) { Line 118: assertFalse(ImageStatus.LOCKED == diskImage.getImageStatus()); minor issue: Why not assertNotEquals that it will be correlated to the assertEquals later in the code Line 119: } Line 120: dao.updateStatusOfImagesByImageGroupId(image.getDiskId(), ImageStatus.LOCKED); Line 121: snapshots = dbFacade.getDiskImageDao().getAllSnapshotsForImageGroup(image.getDiskId()); Line 122: -- 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: 3 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
