Liron Ar has posted comments on this change. Change subject: core: introduce RemoveDiskSnapshotsCommand ......................................................................
Patch Set 10: (10 comments) http://gerrit.ovirt.org/#/c/26327/10/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 31: import org.ovirt.engine.core.compat.Guid; Line 32: import org.ovirt.engine.core.dao.DiskDao; Line 33: import org.ovirt.engine.core.dao.DiskImageDAO; Line 34: import org.ovirt.engine.core.dao.VmDeviceDAO; Line 35: please remember to add the LockIdNameAttribute annotation please add to the non transacative annotation (forceCompensation=true) Line 36: @NonTransactiveCommandAttribute Line 37: public class RemoveDiskSnapshotsCommand<T extends RemoveDiskSnapshotsParameters> extends BaseImagesCommand<T> Line 38: implements TaskHandlerCommand<RemoveDiskSnapshotsParameters> { Line 39: Line 126: return false; Line 127: } Line 128: Line 129: VmValidator vmValidator = createVmValidator(getVm()); Line 130: if (isDiskPlugged() && !validate(vmValidator.vmDown())) { perhaps we can improve the message in that scneario (instead of getting "vm not down" message. Line 131: return false; Line 132: } Line 133: Line 134: if (!validate(new StoragePoolValidator(getStoragePool()).isUp()) || Line 328: Line 329: @Override Line 330: protected Map<String, Pair<String, String>> getExclusiveLocks() { Line 331: return Collections.singletonMap(getImageGroupId().toString(), Line 332: LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK, VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_LOCKED)); message should be changed as well from ACTION_TYPE_ FAILED_VM_IS_LOCKED Line 333: } Line 334: Line 335: @Override Line 336: public Guid createTask(Guid taskId, AsyncTaskCreationInfo asyncTaskCreationInfo, VdcActionType parentCommand) { http://gerrit.ovirt.org/#/c/26327/10/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 22: * Line 23: * @return A {@link org.ovirt.engine.core.bll.ValidationResult} with the validation information. Line 24: */ Line 25: public ValidationResult diskSnapshotsNotExist(List<Guid> imageIds) { Line 26: List<String> disksNotExistInDbIds = new ArrayList<>(); we should also check that the sizes are equal, otherwise if some of the images weren't loaded we'll get VALID. to improve things here, i'd suggest that the validator will get a list of disk images (as of today) in the ctor and will move it to a map of id-entity in the c'tor using Entities.businessEntitiesById that it'll use internally here also. Line 27: for (DiskImage image : images) { Line 28: if (!imageIds.contains(image.getImageId())) { Line 29: disksNotExistInDbIds.add(image.getImageId().toString()); Line 30: } Line 31: } Line 32: Line 33: if (!disksNotExistInDbIds.isEmpty()) { Line 34: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SNAPSHOTS_NOT_EXIST, Line 35: String.format("$diskSnapshotIds %s", StringUtils.join(disksNotExistInDbIds, ", "))); i'd also add to the message the image group id ("snapshots X,Y,Z of disk G .. " Line 36: } Line 37: Line 38: return ValidationResult.VALID; Line 39: } Line 62: if (!diskImage.getActive()) { Line 63: continue; Line 64: } Line 65: Line 66: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_ACTIVE_IMAGE); 1. the message isn't correct, it shouldn't be related to "REMOVE". 2. suggestion - perhaps also add to the message which aren't active (without early return) to provide more info Line 67: } Line 68: Line 69: return ValidationResult.VALID; Line 70: } http://gerrit.ovirt.org/#/c/26327/10/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java: Line 36: import org.ovirt.engine.core.dao.VmTemplateDAO; Line 37: import org.ovirt.engine.core.utils.MockConfigRule; Line 38: Line 39: /** A test case for the {@link RemoveDiskSnapshotsCommand} class. */ Line 40: @RunWith(MockitoJUnitRunner.class) what about more test cases? :) Line 41: public class RemoveDiskSnapshotsCommandTest { Line 42: Line 43: private RemoveDiskSnapshotsCommand<RemoveDiskSnapshotsParameters> cmd; Line 44: Line 110: mockVm(); Line 111: Line 112: vmValidator = spy(new VmValidator(cmd.getVm())); Line 113: doReturn(vmValidator).when(cmd).createVmValidator(any(VM.class)); Line 114: doReturn(ValidationResult.VALID).when(vmValidator).vmNotHavingDeviceSnapshotsAttachedToOtherVms(false); not relevant anymore (removed from the command) Line 115: Line 116: diskImagesValidator = spy(new DiskImagesValidator(mockImages())); Line 117: doReturn(diskImagesValidator).when(cmd).createDiskImageValidator(any(List.class)); Line 118: doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotExist(); Line 145: Line 146: private List<DiskImage> mockImages() { Line 147: DiskImage image1 = new DiskImage(); Line 148: image1.setImageId(IMAGE_ID_1); Line 149: image1.setPlugged(true); unneeded Line 150: image1.setStorageIds(new ArrayList<>(Arrays.asList(STORAGE_DOMAIN_ID))); Line 151: Line 152: DiskImage image2 = new DiskImage(); Line 153: image2.setImageId(IMAGE_ID_2); Line 150: image1.setStorageIds(new ArrayList<>(Arrays.asList(STORAGE_DOMAIN_ID))); Line 151: Line 152: DiskImage image2 = new DiskImage(); Line 153: image2.setImageId(IMAGE_ID_2); Line 154: image2.setPlugged(true); unneeded Line 155: image2.setStorageIds(new ArrayList<>(Arrays.asList(STORAGE_DOMAIN_ID))); Line 156: Line 157: return new ArrayList<>(Arrays.asList(image1, image2)); Line 158: } -- 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: 10 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
