Allon Mureinik has posted comments on this change.
Change subject: core: Remove disk from snapshots (#828192)
......................................................................
Patch Set 18: (10 inline comments)
Answered review - see replies inline.
will send a new path with agreed fixes.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java
Line 115: while (!imageTemplate.equals(currentGuid) &&
!currentGuid.equals(Guid.Empty)) {
pre-existing code, not part of this patch.
will fix in a separate patch.
Line 125:
"RemoveImageCommand::RemoveImageFromDB: 'image' (snapshot of image '{0}') is
null, cannot remove it.",
this is pre-existing code, not part of this patch.
I'll fix it in a separate patch.
Line 159: }
gerrit foobar - in the code itself both "protected" and "}" start on column 5.
Line 161: private List<Snapshot>
prepareSnapshotConfigurationsWithoutImage(Guid imageGroupToRemove) {
I was aiming to align the terminology with Snapshot.getVmConfiguration and
Snapshot.setVmConfiguration, but I agree, this can be shortened to Config.
Line 163: List<DiskImage> snapshotDisks =
getDiskImageDao().getAllSnapshotsForImageGroup(imageGroupToRemove);
bad naming (historically).
RemoveSanpshot and RemoveSnapshotFromDB removes an /disk/ snapshot.
This method, on the other hand, creates a /vm snapshot/ without the disk in it.
Line 176: protected Snapshot
prepareSnapshotConfigurationsWithoutImageSingleImage(Guid vmSnapshotId, Guid
imageId) {
Added javadoc
Line 195: if (imageInList.getImageId().equals(imageId)) {
agree
Line 202: snap.setVmConfiguration(newOvf);
I think it's more readable with the additional local variable - I dislike long
lines with a.doSomething(b.getSomethignElse().computeOn(c)));
Note that there is no performance difference - the String will be created anyway
Line 208: return null;
fair enough
....................................................
Commit Message
Line 11: When a disk is removed all it's snapshots are removed.
Done
--
To view, visit http://gerrit.ovirt.org/6025
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I97dedb9b8e8cd4dd0cae6c7b01d4b5678ba0a2d0
Gerrit-PatchSet: 18
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
