Maor Lipchuk has posted comments on this change. Change subject: core: Add support for copied disk template ......................................................................
Patch Set 7: (5 comments) http://gerrit.ovirt.org/#/c/35933/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateFromConfigurationCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateFromConfigurationCommand.java: Line 112: } Line 113: Line 114: private void setCopiedImages() { Line 115: List<OvfEntityData> ovfEntityDataList = Line 116: getUnregisteredOVFDataDao().getByEntityIdAndStorageDomain(ovfEntityData.getEntityId(), null); > I think that the logic here should be simplified...it's hard for me to revi a. done b. it is already being done c+d. There is no need for a map, I'm already adding the StorageDomainId to the image in a new clonedImageList Line 117: List<DiskImage> copiedTemplateImagesList = initializeListForCopiedImages(); Line 118: for (OvfEntityData ovfEntityDataFetched : ovfEntityDataList) { Line 119: updateCopiedTemplateImages(copiedTemplateImagesList, getImages(), ovfEntityDataFetched.getStorageDomainId()); Line 120: } Line 120: } Line 121: insertNewCopiedDiskImage(copiedTemplateImagesList); Line 122: } Line 123: Line 124: private List<DiskImage> initializeListForCopiedImages() { > Can you elaborate what's being done here? Will change this to clone (Although getImages() is not really crucial since this is the end of the execution which already been commited) the original storage domain should be removed (see line 129) and after that we add all the other Storage Domains for each Storage Domain Line 125: List<DiskImage> clonedDiskImages = new ArrayList<>(getImages()); Line 126: ArrayList<Guid> storageDomains = new ArrayList<>(); Line 127: for (DiskImage clonedDiskImage : clonedDiskImages) { Line 128: clonedDiskImage.setStorageIds(storageDomains); Line 121: insertNewCopiedDiskImage(copiedTemplateImagesList); Line 122: } Line 123: Line 124: private List<DiskImage> initializeListForCopiedImages() { Line 125: List<DiskImage> clonedDiskImages = new ArrayList<>(getImages()); > it's not cloned, same objects are added... see previous answer, changing it to be cloned so the process will be defined properly Line 126: ArrayList<Guid> storageDomains = new ArrayList<>(); Line 127: for (DiskImage clonedDiskImage : clonedDiskImages) { Line 128: clonedDiskImage.setStorageIds(storageDomains); Line 129: } Line 129: } Line 130: return clonedDiskImages; Line 131: } Line 132: Line 133: private void updateCopiedTemplateImages(List<DiskImage> copiedImagesList, > copiedImagesList parameter isn't used it should be used at line 136, fixed. Line 134: List<DiskImage> originaTemplatelmages, Line 135: Guid storageDomainId) { Line 136: List<DiskImage> clonedTemplateDiskImages = new ArrayList<>(getImages()); Line 137: List<Guid> imagesContainedInStorageDomain = getImagesGuidFromStorage(storageDomainId, getStoragePoolId()); Line 153: private List<Guid> getImagesGuidFromStorage(Guid storageDomainId, Guid storagePoolId) { Line 154: List imagesList = new ArrayList<>(); Line 155: try { Line 156: VDSReturnValue imagesListResult = runVdsCommand(VDSCommandType.GetImagesList, Line 157: new GetImagesListVDSCommandParameters(storageDomainId, storagePoolId)); > you should add a map to cache the results of the domains to avoid query it it is already being done every time for a different Storage Domain Line 158: if (imagesListResult.getSucceeded()) { Line 159: imagesList = (List<Guid>) imagesListResult.getReturnValue(); Line 160: } else { Line 161: log.error("Unable to get images list for storage domain, can not update copied template disks realted to Storage Domain id '{}'", -- To view, visit http://gerrit.ovirt.org/35933 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7514146d26792a361e47d1de820f5233ff9bf40 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@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: 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
