Maor Lipchuk has posted comments on this change.
Change subject: core: fail deletion of template disk with derived disks
......................................................................
Patch Set 2:
(3 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java
Line 200: private DiskImagesValidator createDiskImagesValidator(DiskImage
disk) {
Line 201: return new DiskImagesValidator(Arrays.asList(disk));
Line 202: }
Line 203:
Line 204: private boolean checkDerivedDisksFromDiskNotExist(DiskImage
diskImage) {
isTemplateDiskHasDerivedDisks?
Line 205: return
validate(createDiskImagesValidator(diskImage).diskImagesHaveNoDerivedDisks(getParameters().getStorageDomainId()));
Line 206: }
Line 207:
Line 208: private List<String> getNamesOfDerivedVmsFromTemplate(DiskImage
diskImage) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskImagesValidator.java
Line 130: }
Line 131:
Line 132:
Line 133: public ValidationResult diskImagesHaveNoDerivedDisks(Guid
storageDomainId) {
Line 134: List<String> disksInfo = null;
I didn't saw it was discussed here.
any how, performance wise I'm not sure if calling an if condition at every disk
in the loop will be better, also the code looks much simpler to read... but
that's simply my opinion
Line 135: for (DiskImage diskImage : diskImages) {
Line 136: if (diskImage.getVmEntityType() != null &&
diskImage.getVmEntityType().isTemplateType()) {
Line 137: List<DiskImage> basedDisks =
getDiskImageDAO().getAllSnapshotsForParent(diskImage.getImageId());
Line 138: for (DiskImage basedDisk : basedDisks) {
Line 132:
Line 133: public ValidationResult diskImagesHaveNoDerivedDisks(Guid
storageDomainId) {
Line 134: List<String> disksInfo = null;
Line 135: for (DiskImage diskImage : diskImages) {
Line 136: if (diskImage.getVmEntityType() != null &&
diskImage.getVmEntityType().isTemplateType()) {
1. but you're validation is for template disks, how can a template disk be
floating?
2. When you will have derived disks which are not from template disks? why
should we consider checking this?
I think the main question is, what do you think will use this validation? I
think that this validation is only relevant to template disks, and any other
use of it will be wrong and should not be return a valid status. Am I'm missing
anything?
Line 137: List<DiskImage> basedDisks =
getDiskImageDAO().getAllSnapshotsForParent(diskImage.getImageId());
Line 138: for (DiskImage basedDisk : basedDisks) {
Line 139: if (storageDomainId == null ||
basedDisk.getStorageIds().contains(storageDomainId)) {
Line 140: if (disksInfo == null) {
--
To view, visit http://gerrit.ovirt.org/22428
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifca48993f40f3c5f7b603f33add7049e78e6c4e9
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
