Mike Kolesnik has posted comments on this change.
Change subject: [DO NOT SUBMIT] core: Migrate snapshots data & start using table
......................................................................
Patch Set 3: (30 inline comments)
Answered some comments inline
....................................................
File
backend/manager/dbscripts/upgrade/03_01_0600_migrate_images_to_snapshots.sql
Line 97: /* UPDATE snapshots
Actually no, forgot to uncomment.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskToVmCommand.java
Line 287:
parameters.setVmSnapshotId(DbFacade.getInstance().getSnapshotDao().getId(getVmId(),
SnapshotType.ACTIVE));
Done
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 76: if (getDisksList().isEmpty()) {
Can do action doesn't fail if there are no disks..
If there are no disks at this point then we need to finish the command
gracefully.
Line 77: EndVmCommand();
Done
Line 103: setSucceeded(true);
Done
Line 103: setSucceeded(true);
Done
Line 125:
getSnapshotDao().updateId(getSnapshotDao().getId(getVmId(),
SnapshotType.ACTIVE), createdSnapshotId);
The "new" active snapshot is irrelevant in this case since we revert the images
to the old images, so the removed snapshot is actually what needs to go back to
being active, as it was the active snapshot before.
Line 125:
getSnapshotDao().updateId(getSnapshotDao().getId(getVmId(),
SnapshotType.ACTIVE), createdSnapshotId);
The "new" active snapshot is irrelevant in this case since we revert the images
to the old images, so the removed snapshot is actually what needs to go back to
being active, as it was the active snapshot before.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateSnapshotCommand.java
Line 136:
image.setvm_snapshot_id(DbFacade.getInstance().getSnapshotDao().getId((Guid)
getParameters().getEntityId(), SnapshotType.ACTIVE));
Currently this class has no test, so no need for premature optimization..
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java
Line 263:
DbFacade.getInstance().getSnapshotDao().exists(getVmId(),
SnapshotType.STATELESS)) {
1. Currently this class has no test, so no need for premature optimization..
2. It is not a shared hierarchy.
Line 263:
DbFacade.getInstance().getSnapshotDao().exists(getVmId(),
SnapshotType.STATELESS)) {
1. Currently this class has no test, so no need for premature optimization..
2. It is not a shared hierarchy.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java
Line 95:
DbFacade.getInstance().getSnapshotDao().remove(getParameters().getSnapshotId());
Same comment, no test - no need
Line 95:
DbFacade.getInstance().getSnapshotDao().remove(getParameters().getSnapshotId());
Same comment, no test - no need
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 92:
It's done in the runAsyncTask method
Line 105: for (Guid snapshotId : snapshotsToRemove) {
Yes it's roll forward
Line 159: SnapshotStatus.OK);
Yes it's very similar behavior
Line 159: SnapshotStatus.OK);
Yes it's very similar behavior
Line 159: SnapshotStatus.OK);
Yes it's very similar behavior
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java
Line 36: Guid snapshotId =
DbFacade.getInstance().getSnapshotDao().getId(getVmId(),
SnapshotType.STATELESS);
Currently this class has no test, so no need for premature optimization..
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java
Line 246: &&
DbFacade.getInstance().getSnapshotDao().exists(getVm().getId(),
SnapshotType.STATELESS)) {
Done
Line 285: if
(DbFacade.getInstance().getSnapshotDao().exists(getVm().getId(),
SnapshotType.STATELESS)) {
Done
Line 285: if
(DbFacade.getInstance().getSnapshotDao().exists(getVm().getId(),
SnapshotType.STATELESS)) {
Done
Line 891: private void SetIsVmRunningStateless() {
This is unrelated change so i would rather not do it in this patch
Line 892: _isVmRunningStateless =
DbFacade.getInstance().getSnapshotDao().exists(getVmId(),
SnapshotType.STATELESS);
Execute is not tested ATM so no need for premature optimization
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 92: Guid newActiveSnapshot = Guid.NewGuid();
Done
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java
Line 86:
Done
Line 86:
Done
Line 245:
Done
Line 245:
Done
Line 245:
Done
--
To view, visit http://gerrit.ovirt.org/2314
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fa10e27736b05cf95fac22d088156b83de0747c
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Muli Salem <msa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
