Idan Shaby has posted comments on this change. Change subject: core: WAD should be Ignored on File Domain Disks ......................................................................
Patch Set 1: (5 comments) http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskCommand.java: Line 47: protected VDSReturnValue mergeSnapshots() { Line 48: boolean wipeAfterDelete = ImagesHandler.shouldWipeAfterDelete( Line 49: getDiskImage().isWipeAfterDelete(), getStorageDomain().getStorageType()); Line 50: MergeSnapshotsVDSCommandParameters params = new MergeSnapshotsVDSCommandParameters(getStoragePoolId(), Line 51: getStorageDomainId(), getVmId(), getDiskImage().getId(), getDiskImage().getImageId(), > I wouldn't change the getXXXId() calls - you already have a local variable, Note that I used the getXXXId() calls in another function, and that they contain logic which takes care of null issues etc. So maybe it should not change? Line 52: getDestinationDiskImage().getImageId(), wipeAfterDelete); Line 53: return runVdsCommand(VDSCommandType.MergeSnapshots, params); Line 54: } Line 55: http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java: Line 327: new Predicate<Disk>() { Line 328: @Override Line 329: public boolean eval(Disk disk) { Line 330: return ImagesHandler.shouldWipeAfterDelete( Line 331: disk.isWipeAfterDelete(), getStorageDomain().getStorageType()); > This is wrong. That is true, but note that guids.get(0) gives you the memory volumes' storage domain, and not the disks'. Anyway, setStorageDomainId(guids.get(0)) is wrong here, as you commented in AddVmAndCloneImageCommand.java, so I fixed it. Line 332: } Line 333: }).size() > 0; Line 334: Line 335: Guid taskId1 = persistAsyncTaskPlaceHolder(parentCommand, DELETE_PRIMARY_IMAGE_TASK_KEY); http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/HibernationVolumesRemover.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/HibernationVolumesRemover.java: Line 49: enclosingCommand.getParameters().getParentCommand()); Line 50: } Line 51: Line 52: protected boolean isPostZero(List<Guid> guids) { Line 53: return isPostZero(getDiskDao().getAllForVm(vmId), getStorageDomainId(guids)); > why stop caching? I didn't. Instead of doing it identically three times in HibernationVolumesRemover, MemoryImageRemoverOnDataDomain and MemoryImageRemoverFromExportDomain, I did it only once in MemoryImageRemover -> isPostZero. Line 54: } http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverFromExportDomain.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverFromExportDomain.java: Line 46: guids.get(1), getStorageDomainId(guids), guids.get(4), isPostZero(guids), false); Line 47: } Line 48: Line 49: protected boolean isPostZero(List<Guid> guids) { Line 50: return isPostZero(vm.getDiskMap().values(), getStorageDomainId(guids)); > why stop caching? I didn't. Instead of doing it identically three times in HibernationVolumesRemover, MemoryImageRemoverOnDataDomain and MemoryImageRemoverFromExportDomain, I did it only once in MemoryImageRemover -> isPostZero. Line 51: } http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java: Line 39: guids.get(1), getStorageDomainId(guids), guids.get(4), isPostZero(guids), false); Line 40: } Line 41: Line 42: protected boolean isPostZero(List<Guid> guids) { Line 43: return isPostZero(getDiskDao().getAllForVm(vmId), getStorageDomainId(guids)); > why stop caching? I didn't. Instead of doing it identically three times in HibernationVolumesRemover, MemoryImageRemoverOnDataDomain and MemoryImageRemoverFromExportDomain, I did it only once in MemoryImageRemover -> isPostZero. Line 44: } Line 45: Line 46: @Override Line 47: protected boolean isMemoryStateRemovable(String memoryVolume) { -- To view, visit http://gerrit.ovirt.org/30706 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie8a9932725ef861525f5102e8617938f0e0a71c7 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
