Liron Aravot 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 review it at the moment - a. if the template has no disks, return immediately b. get the unregistered ovf data - get the domains out of it c. for each domain issue getImageList/GetUnregisteredDisksQuery and save the results in a map. d. go over the template disks, for each disk check if it contains in the values of the map created in C and if it is add the domain to its domain list. right now i find it hard to review and i think that there are some issues here...this is clearer and easier imo. 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? a list containing the same elements of getImages() is created which seems wrong . all the disks in that list are assigned with the same storageDomains list, which is wrong as well, all of them are with the same list so changing the domains of one will affect the other. perhaps i missed the point,,can you explain this method? 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... 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 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 each tinme 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
