Tomas Jelinek has posted comments on this change. Change subject: core: Allow creation of new disks as copy of any existing disk ......................................................................
Patch Set 3: (10 comments) ad generic comments: - the translated properties are removed than in the subsequent FE patch - the quota is handled in the getQuotaStorageConsumptionParameters() - same way as it was for copy template disk. Is something special missing in this context? https://gerrit.ovirt.org/#/c/38334/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java: Line 134 Line 135 Line 136 Line 137 Line 138 > This is not relevant anymore, please remove the comment done Line 140 Line 141 Line 142 Line 143 Line 144 > How can we be sure that the disk is not floating? this works for floating disks as well - any reason to disallow it? Line 358 Line 359 Line 360 Line 361 Line 362 > Why removing this? What if we will try to copy a disk which is floating fro you can do it also from webadmin. Why is it a problem? Line 181: protected boolean checkIfNeedToBeOverride() { Line 182: if (isTemplate() && Line 183: getParameters().getOperation() == ImageOperation.Copy && Line 184: !getParameters().getForceOverride() && Line 185: getImage().getStorageIds().contains(getStorageDomain().getId())) { > Please consider to extract this to a separate method not sure if it would help readability - how would you call it so it will be more obvious than it is now? Line 186: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_ALREADY_EXISTS); Line 187: } Line 188: return true; Line 189: } Line 337: * The following method will override a parameters which are not relevant for the MoveOrCopyDiskCommand to the Line 338: * correct values for these scenario in order to be used at parent class Line 339: */ Line 340: private void overrideParameters() { Line 341: boolean isTemplate = isTemplate(); > style: It is pretty confusing calling the variable with the same name of th Done Line 342: Line 343: if (getParameters().getOperation() == ImageOperation.Copy) { Line 344: getParameters().setUseCopyCollapse(true); Line 345: getParameters().setAddImageDomainMapping(true); Line 346: Line 347: if (!isTemplate) { Line 348: getParameters().setAddImageDomainMapping(false); Line 349: Line 350: Guid newId = Guid.newGuid(); > please rename this variable to newImageId Done Line 351: Guid newGroupId = Guid.newGuid(); Line 352: Line 353: DiskImage image = getImage(); Line 354: Line 347: if (!isTemplate) { Line 348: getParameters().setAddImageDomainMapping(false); Line 349: Line 350: Guid newId = Guid.newGuid(); Line 351: Guid newGroupId = Guid.newGuid(); > Please call this newId, since we don't really use group id any more, only i Done Line 352: Line 353: DiskImage image = getImage(); Line 354: Line 355: image.setId(newGroupId); Line 350: Guid newId = Guid.newGuid(); Line 351: Guid newGroupId = Guid.newGuid(); Line 352: Line 353: DiskImage image = getImage(); Line 354: > redundant empty line Done Line 355: image.setId(newGroupId); Line 356: image.setImageId(newId); Line 357: Line 358: image.setDiskAlias(getDiskAlias()); Line 361: image.setQuotaId(getParameters().getQuotaId()); Line 362: image.setDiskProfileId(getParameters().getDiskProfileId()); Line 363: Line 364: getParameters().setDestinationImageId(newId); Line 365: getParameters().setImageGroupID(getParameters().getImageGroupID()); > I don't understand this line, you are setting the parameter which you are g hmmm, not sure what evolution moved me to this state but it does not make sense anymore - removed Line 366: getParameters().setDestImageGroupId(newGroupId); Line 367: } Line 368: } else { Line 369: getParameters().setUseCopyCollapse(false); Line 362: image.setDiskProfileId(getParameters().getDiskProfileId()); Line 363: Line 364: getParameters().setDestinationImageId(newId); Line 365: getParameters().setImageGroupID(getParameters().getImageGroupID()); Line 366: getParameters().setDestImageGroupId(newGroupId); > Please think about refactor this to a separate method Done Line 367: } Line 368: } else { Line 369: getParameters().setUseCopyCollapse(false); Line 370: } -- To view, visit https://gerrit.ovirt.org/38334 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07eece05b1880011f1a1b4300d07f0bd1ebf30bd Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <tjeli...@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: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
