Allon Mureinik has posted comments on this change.
Change subject: core: errors during preview of diskless snapshots
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
Mainly questions - marked with (-1) to avoid accidental pushing before answers
are given.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 121:
Line 122: final Guid newActiveSnapshotId = Guid.NewGuid();
Line 123: final List<DiskImage> images = DbFacade
Line 124: .getInstance()
Line 125: .getDiskImageDao()
use getDiskImageDao()
Line 126:
.getAllSnapshotsForVmSnapshot(getParameters().getDstSnapshotId());
Line 127: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 128: @Override
Line 129: public Void runInTransaction() {
Line 135: "Active VM before the preview",
Line 136: SnapshotType.PREVIEW,
Line 137: getVm(),
Line 138: getCompensationContext());
Line 139:
snapshotsManager.addActiveSnapshot(newActiveSnapshotId, getVm(),
getCompensationContext());
I wonder if we should allow passing a null compensation context, so if we know
we aren't going to call stateChanged, we won't even register the compensation
data.
Line 140:
snapshotsManager.removeAllIllegalDisks(previousActiveSnapshotId,
getVm().getId());
Line 141: // if there are no images we can should restore the
config now so it'll be executed within the transaction.
Line 142: if (!images.isEmpty()) {
Line 143: getCompensationContext().stateChanged();
Line 137: getVm(),
Line 138: getCompensationContext());
Line 139:
snapshotsManager.addActiveSnapshot(newActiveSnapshotId, getVm(),
getCompensationContext());
Line 140:
snapshotsManager.removeAllIllegalDisks(previousActiveSnapshotId,
getVm().getId());
Line 141: // if there are no images we can should restore the
config now so it'll be executed within the transaction.
s/can should/should/
Line 142: if (!images.isEmpty()) {
Line 143: getCompensationContext().stateChanged();
Line 144: } else {
Line 145: restoreVmConfigFromSnapshot();
Line 141: // if there are no images we can should restore the
config now so it'll be executed within the transaction.
Line 142: if (!images.isEmpty()) {
Line 143: getCompensationContext().stateChanged();
Line 144: } else {
Line 145: restoreVmConfigFromSnapshot();
shouldn't we also free the lock?
Line 146: }
Line 147: return null;
Line 148: }
Line 149: });
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
Line 251:
Line 252: public static void updateDisksFromDb(VM vm) {
Line 253: List<Disk> imageList =
DbFacade.getInstance().getDiskDao().getAllForVm(vm.getId());
Line 254: vm.getDiskList().clear();
Line 255: vm.getDiskMap().clear();
Please add a javadoc do explain this behaviour.
Also - this change will affect a couple of dozens of calls.
Are we sure we didn't introduce any regressions here?
Line 256: updateDisksForVm(vm, imageList);
Line 257: }
Line 258:
Line 259: public static void updateDisksForVm(VM vm, List<? extends Disk>
diskList) {
--
To view, visit http://gerrit.ovirt.org/9128
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e8edc74bc34676f526dfd24d2f89eb60d8acf2e
Gerrit-PatchSet: 2
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: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
