Maor Lipchuk has posted comments on this change. Change subject: exporting OvfDataUpdater logic to commands ......................................................................
Patch Set 4: (12 comments) http://gerrit.ovirt.org/#/c/33157/4//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-09-22 10:13:48 +0300 Line 4: Commit: Liron Aravot <lara...@redhat.com> Line 5: CommitDate: 2014-10-26 14:57:05 +0200 Line 6: Line 7: exporting OvfDataUpdater logic to commands Please add prefix of the component type (core: I assume) Line 8: Line 9: This patch movess the ovf update logic resides within the Line 10: OvfDataUpdater to BLL commands, so that it can be executed easily by Line 11: other flows as well. Line 5: CommitDate: 2014-10-26 14:57:05 +0200 Line 6: Line 7: exporting OvfDataUpdater logic to commands Line 8: Line 9: This patch movess the ovf update logic resides within the /s/movess/moves Line 10: OvfDataUpdater to BLL commands, so that it can be executed easily by Line 11: other flows as well. Line 12: Line 13: The logic was mainly moved to ProcessOvfUpdateForStoragePoolCommand, 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(); 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) { > it's not a domain, but the list of the domains return from the executed act You can only get empty Set, I don;t think this validation is redundant, no? 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 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? 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 86: logInfoIfNeeded(pool, "Attempting to update template OVFs in Data Center {0}", Line 87: pool.getName()); Line 88: Line 89: updateOvfForTemplatesOfStoragePool(pool); Line 90: please remove redundant empty line, or add a comment before Line 91: logInfoIfNeeded(pool, "Successfully updated templates OVFs in Data Center {0}", Line 92: pool.getName()); Line 93: logInfoIfNeeded(pool, "Attempting to remove unneeded template/vm OVFs in Data Center {0}", Line 94: pool.getName()); 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 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 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 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 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, AuditLogableBase should not be acting like a DAO helper. either get the OVF to implement this methods or use simplt getDbFacade() 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
