Muli Salem has posted comments on this change.
Change subject: [DO NOT SUBMIT] core: Migrate snapshots data & start using table
......................................................................
Patch Set 3: (23 inline comments)
....................................................
File
backend/manager/dbscripts/upgrade/03_01_0600_migrate_images_to_snapshots.sql
Line 22: AND v.entity_type = 'VM'))
Please remove trailing white space.
Line 97: /* UPDATE snapshots
Should this be removed?
....................................................
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));
For test purposes, perhaps extract to a private method with:
return DbFacade.getInstance().getSnapshotDao();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 76: if (getDisksList().isEmpty()) {
This is already checked in the canDoAction, consider removing.
Line 77: EndVmCommand();
Wouldn't it be better to call it EndSuccessfully() here (and in override below)
?
Line 103: setSucceeded(true);
No reason to set inside the for loop.
Line 125:
getSnapshotDao().updateId(getSnapshotDao().getId(getVmId(),
SnapshotType.ACTIVE), createdSnapshotId);
Why update with the removed createdSnapshotId ?
....................................................
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));
For test purposes, perhaps extract to a private method with:
return DbFacade.getInstance().getSnapshotDao();
Line 168:
DbFacade.getInstance().getDiskImageDAO().update(getDiskImage());
same comment.
Line 169: DbFacade.getInstance()
same comment.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java
Line 263:
DbFacade.getInstance().getSnapshotDao().exists(getVmId(),
SnapshotType.STATELESS)) {
Same comment - perhaps put the method higher up in the 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, for test purposes.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 92:
Shouldn't you set the parent command here ?
Line 105: for (Guid snapshotId : snapshotsToRemove) {
What happens if the first image is removed successfully above, and the second
image isn't removed properly? We will still reach this code block, and remove
the snaphot from the DB.
Line 159: SnapshotStatus.OK);
Fall through on purpose ?
....................................................
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);
extract to private method for test purposes.
....................................................
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)) {
Extract to private method (probably higher up).
Line 285: if
(DbFacade.getInstance().getSnapshotDao().exists(getVm().getId(),
SnapshotType.STATELESS)) {
Exctract....
Line 891: private void SetIsVmRunningStateless() {
Please change method to start with lowercase.
Line 892: _isVmRunningStateless =
DbFacade.getInstance().getSnapshotDao().exists(getVmId(),
SnapshotType.STATELESS);
Consider extracting for test purposes.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 92: Guid newActiveSnapshot = Guid.NewGuid();
Consider changing to newActiveSnapshotId
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java
Line 86:
Double space can be removed.
Line 245:
Remove space.
--
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
