Liron Aravot has posted comments on this change. Change subject: exporting OvfDataUpdater logic to commands ......................................................................
Patch Set 4: (9 comments) http://gerrit.ovirt.org/#/c/33157/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java: Line 65: log.info("Attempting to update VMs/Templates Ovf."); Line 66: for (StoragePool pool : storagePools) { Line 67: VdcReturnValueBase returnValueBase = performOvfUpdateForStoragePool(pool.getId()); Line 68: if (!returnValueBase.getSucceeded()) { Line 69: log.errorFormat("Exception while trying to update or remove VMs/Templates ovf in Data Center {0}.", pool.getName()); > you forgot to add continue(); i don't think that it's needed, if we somehow failed, let's still update the ovf stores with the existing data. Line 70: } Line 71: Line 72: if (ovfOnAnyDomainSupported(pool)) { Line 73: log.infoFormat("Attempting to update ovfs in domain in Data Center {0}", Line 73: log.infoFormat("Attempting to update ovfs in domain in Data Center {0}", Line 74: pool.getName()); Line 75: Line 76: Set<Guid> domainsToUpdate = (Set<Guid>) returnValueBase.getActionReturnValue(); Line 77: if (domainsToUpdate != null) { > You can only get empty Set, I don;t think this validation is redundant, no? it shouldn't happen, but we can't rely on that here. I added log.error in case of null so we'll know what went wrong. Line 78: for (Guid id : domainsToUpdate) { Line 79: performOvfUpdateForDomain(pool.getId(), id); Line 80: } Line 81: } http://gerrit.ovirt.org/#/c/33157/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfUpdateProcessHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfUpdateProcessHelper.java: Line 30: private OvfManager ovfManager; Line 31: Line 32: public OvfUpdateProcessHelper() { Line 33: ovfManager = new OvfManager(); Line 34: } > Please add an empty line here Done Line 35: /** Line 36: * Adds the given vm metadata to the given map Line 37: */ Line 38: protected String buildMetadataDictionaryForVm(VM vm, http://gerrit.ovirt.org/#/c/33157/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java: Line 105 Line 106 Line 107 Line 108 Line 109 > How is this related to this patch? it's related because it was moved higher in the hirerchy as i need it in other command, so no need to override it with same implementation http://gerrit.ovirt.org/#/c/33157/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessOvfUpdateForStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessOvfUpdateForStoragePoolCommand.java: Line 141: } Line 142: Line 143: /** Line 144: * Update ovfs for updated/newly vms since last run for the given storage pool Line 145: * > please remove empty line it's just copied, i prefer to not make changes here Line 146: */ Line 147: protected void updateOvfForVmsOfStoragePool(StoragePool pool) { Line 148: Guid poolId = pool.getId(); Line 149: List<Guid> vmsIdsForUpdate = getVmAndTemplatesGenerationsDAO().getVmsIdsForOvfUpdate(poolId); Line 163: Line 164: /** Line 165: * Removes from the storage ovf files of vm/templates that were removed from the db since the last OvfDataUpdater Line 166: * run. Line 167: * > please remove empty line same Line 168: */ Line 169: protected void removeOvfForTemplatesAndVmsOfStoragePool(StoragePool pool) { Line 170: Guid poolId = pool.getId(); Line 171: removedOvfIdsInfo = getVmAndTemplatesGenerationsDAO().getIdsForOvfDeletion(poolId); Line 187: } Line 188: Line 189: /** Line 190: * Perform vdsm call which performs ovf update for the given metadata map Line 191: * > please remove empty line same Line 192: */ Line 193: protected void performOvfUpdate(StoragePool pool, Line 194: Map<Guid, KeyValuePairCompat<String, List<Guid>>> vmsAndTemplateMetadata) { Line 195: if (!ovfOnAnyDomainSupported(pool)) { http://gerrit.ovirt.org/#/c/33157/4/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java: Line 653: public VmNetworkInterfaceDao getVmNetworkInterfaceDao() { Line 654: return DbFacade.getInstance().getVmNetworkInterfaceDao(); Line 655: } Line 656: Line 657: public SnapshotDao getVmNetwork() { > You forgot the suffix of Dao here, also the name is wrong I think Done Line 658: return getDbFacade().getSnapshotDao(); Line 659: } Line 660: Line 661: public VmDynamicDAO getVmDynamicDAO() { Line 656: Line 657: public SnapshotDao getVmNetwork() { Line 658: return getDbFacade().getSnapshotDao(); Line 659: } Line 660: > I do not think this should be added here, there are pleny of daos here, when project wide decision will be made to move them from here, all will be removed together.. Line 661: public VmDynamicDAO getVmDynamicDAO() { Line 662: return getDbFacade().getVmDynamicDao(); Line 663: } Line 664: -- To view, visit http://gerrit.ovirt.org/33157 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6780aedf2d215d08c4bed0ff4f7520c45f753f66 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@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
