Daniel Erez has posted comments on this change. Change subject: core: introduce RemoveDiskSnapshotsCommand ......................................................................
Patch Set 9: (13 comments) http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotTaskHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotTaskHandler.java: Line 113: private void updateImagesStatus(ImageStatus imageStatus) { Line 114: Guid imageGroupId = DbFacade.getInstance().getDiskImageDao().getSnapshotById(imageId).getId(); Line 115: ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(imageGroupId, Line 116: imageStatus, Line 117: ImageStatus.OK , > see above comment, should be illegal Done Line 118: enclosingCommand.getCompensationContext()); Line 119: } Line 120: Line 121: @Override http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java: Line 29: import org.ovirt.engine.core.common.utils.Pair; Line 30: import org.ovirt.engine.core.compat.Guid; Line 31: import org.ovirt.engine.core.dao.DiskDao; Line 32: import org.ovirt.engine.core.dao.DiskImageDAO; Line 33: > 1. please add nontransactiveattribute 1. done 2. replaced with memory lock on disk 3. will be prevented by lock Line 34: public class RemoveDiskSnapshotsCommand<T extends RemoveDiskSnapshotsParameters> extends BaseImagesCommand<T> Line 35: implements TaskHandlerCommand<RemoveDiskSnapshotsParameters> { Line 36: Line 37: private List<DiskImage> images; Line 102: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 103: } Line 104: Line 105: DiskSnapshotsValidator diskSnapshotsValidator = createDiskSnapshotsValidator(getParameters().getImageIds()); Line 106: if (!validate(diskSnapshotsValidator.diskSnapshotsNotExist())) { > as we are the only users of that validator, i'd suggest to pass to it the I think the current validator can be still useful for other use-cases. I'll see if I can overload it. Line 107: return false; Line 108: } Line 109: Line 110: DiskImagesValidator diskImagesValidator = createDiskImageValidator(getImages()); Line 107: return false; Line 108: } Line 109: Line 110: DiskImagesValidator diskImagesValidator = createDiskImageValidator(getImages()); Line 111: if (!validate(diskImagesValidator.diskImagesBelongToSameImageGroup())) { > seems to me like this validation should be moved to the disksnapshotvalidat Done Line 112: return false; Line 113: } Line 114: Line 115: if (!validate(diskImagesValidator.diskImagesSnapshotsNotAttachedToOtherVms(false))) { Line 128: if (!validate(new StoragePoolValidator(getStoragePool()).isUp()) || Line 129: !validateVmNotDuringSnapshot() || Line 130: !validateVmNotInPreview() || Line 131: !validateSnapshotExists() || Line 132: !validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms(false)) || > this should be removed, we checked on line 115 that snapshots of that disk Done Line 133: !validateStorageDomainActive()) { Line 134: return false; Line 135: } Line 136: Line 133: !validateStorageDomainActive()) { Line 134: return false; Line 135: } Line 136: Line 137: // Check images > this comment isn't that useful :) Done Line 138: if (!validateImages()) { Line 139: return false; Line 140: } Line 141: Line 139: return false; Line 140: } Line 141: Line 142: // check that we are not deleting the vm working snapshot Line 143: if (!validateImagesNotActive()) { > i'd suggest moving this check up Done Line 144: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_ACTIVE_IMAGE); Line 145: } Line 146: Line 147: if (!validateStorageDomainAvailableSpace()) { Line 262: return validate(diskImagesValidator.diskImagesNotLocked()) && Line 263: validate(diskImagesValidator.diskImagesNotIllegal()); Line 264: } Line 265: Line 266: protected boolean validateImagesNotActive() { > no need for that, we can check the image.isActive(). I'd suggest to have th Done Line 267: for (Guid imageId : getParameters().getImageIds()) { Line 268: if (getDiskImageDao().get(imageId) != null) { Line 269: return false; Line 270: } Line 292: return getImages().get(0); Line 293: } Line 294: Line 295: protected boolean isDiskPlugged() { Line 296: return getDiskDao().getAllForVm(getVmId(), true).contains(getRepresentativeImage()); > it'll be better to load just the needed vm device rather than loading all t Done Line 297: } Line 298: Line 299: @Override Line 300: public List<PermissionSubject> getPermissionCheckSubjects() { http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskSnapshotsValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskSnapshotsValidator.java: Line 31: } Line 32: } Line 33: Line 34: if (!disksNotExistInDbIds.isEmpty()) { Line 35: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_NOT_EXIST, > not sure that the message fits here Done Line 36: String.format("$diskIds %s", StringUtils.join(disksNotExistInDbIds, ", "))); Line 37: } Line 38: Line 39: return ValidationResult.VALID; http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java: Line 204: sumOfActualSizes += (long) image.getActualSize(); Line 205: } Line 206: Line 207: // Finding the max virtual size Line 208: long maxVirtualSize = Collections.max(diskImages, new Comparator<DiskImage>() { > i'd find that during the first loop rather than iterate again. Done Line 209: @Override Line 210: public int compare(DiskImage diskImage1, DiskImage diskImage2) { Line 211: return diskImage1.getSize() < diskImage2.getSize() ? -1 Line 212: : diskImage1.getSize() > diskImage2.getSize() ? 1 : 0; Line 212: : diskImage1.getSize() > diskImage2.getSize() ? 1 : 0; Line 213: } Line 214: }).getSize(); Line 215: Line 216: return isDomainHasSpaceForRequest(Math.min(maxVirtualSize, sumOfActualSizes), false); > shouldn't this be Math.max? (at least it seems so from the method comment) Rephrased comment... Line 217: } http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveDiskSnapshotsParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveDiskSnapshotsParameters.java: Line 9: public class RemoveDiskSnapshotsParameters extends ImagesContainterParametersBase implements Serializable { Line 10: Line 11: private static final long serialVersionUID = -629355522841577585L; Line 12: Line 13: private List<Guid> imageIds; > is that being serialized? if so, it should fail on the json de-serializatio Done Line 14: Line 15: public RemoveDiskSnapshotsParameters(Guid imageId) { Line 16: this(Arrays.asList(imageId)); Line 17: } -- To view, visit http://gerrit.ovirt.org/26327 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia714a4390d1d9b672005be30f58b7fa98b9a31cd Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
