Liron Ar has posted comments on this change. Change subject: core: adding DAO support for unregistered VMs/Templates. ......................................................................
Patch Set 15: (6 comments) looking good, minor comments. http://gerrit.ovirt.org/#/c/26480/15/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/OvfEntityData.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/OvfEntityData.java: Line 4: Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: import org.ovirt.engine.core.compat.Version; Line 7: Line 8: public class OvfEntityData extends IVdcQueryable implements Serializable { please do not inherit from IVdcQueryable unless you override the getQueryableId method. Line 9: private static final long serialVersionUID = 3376648147702972152L; Line 10: Line 11: private Guid entityId; Line 12: private String entityName; Line 78: public void setOvfData(String ovfData) { Line 79: this.ovfData = ovfData; Line 80: } Line 81: Line 82: public String getOvfExtraData() { what is ovf extra data? Line 83: return ovfExtraData; Line 84: } Line 85: Line 86: public void setOvfExtraData(String ovfExtraData) { http://gerrit.ovirt.org/#/c/26480/15/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java: Line 187: put(VmInit.class, VmInitDAO.class); Line 188: put(CpuStatistics.class, VdsCpuStatisticsDAO.class); Line 189: put(VdsNumaNode.class, VdsNumaNodeDAO.class); Line 190: put(VmNumaNode.class, VmNumaNodeDAO.class); Line 191: put(OvfEntityData.class, UnregisteredOVFDataDAO.class); afaik this map is used for the compensation, as this dao isn't used with compensation there's no need to put it here. please remove if unneeded. Line 192: } Line 193: }; Line 194: Line 195: private JdbcTemplate jdbcTemplate; http://gerrit.ovirt.org/#/c/26480/15/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/UnregisteredOVFDataDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/UnregisteredOVFDataDAO.java: Line 31: Line 32: /** Line 33: * Insert new entity to the unregistered table. Line 34: */ Line 35: public void insertOVFDataForEntities(OvfEntityData ovfEntityData); > conventions - please call this method "save" you can also remove "ForEntities", it leads to think that it saves multiple OvfEntityData objects. Line 36: Line 37: /** Line 38: * Remove an entity from the unregistered table. Line 39: * http://gerrit.ovirt.org/#/c/26480/15/packaging/dbscripts/upgrade/03_05_0490_unregistered_ovf_of_entities.sql File packaging/dbscripts/upgrade/03_05_0490_unregistered_ovf_of_entities.sql: Line 1: CREATE TABLE unregistered_ovf_of_entities Line 2: ( Line 3: entity_guid UUID, Line 4: entity_name VARCHAR(255) NOT NULL, Line 5: entity_type VARCHAR(32) NOT NULL, 32 seems a bit big, is it the value that is used for that on other tables as well? Line 6: architecture INTEGER, Line 7: lowest_comp_version VARCHAR(40), Line 8: storage_domain_id UUID, Line 9: ovf_data TEXT, Line 9: ovf_data TEXT, Line 10: ovf_extra_data TEXT, Line 11: CONSTRAINT pk_entity_guid_storage_domain_unregistered PRIMARY KEY(entity_guid, storage_domain_id) Line 12: ); Line 13: we can have an index for storage domain id/entity type. -- To view, visit http://gerrit.ovirt.org/26480 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I385fac757f46131ae0c0048b6cf39b78f037e852 Gerrit-PatchSet: 15 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
