Maor Lipchuk has posted comments on this change. Change subject: core: adding DAO support for unregistered VMs/Templates. ......................................................................
Patch Set 11: (11 comments) http://gerrit.ovirt.org/#/c/26480/11/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 8: Line 9: public class OvfEntityData extends IVdcQueryable implements Serializable { Line 10: private static final long serialVersionUID = 3376648147702972152L; Line 11: Line 12: private Guid vmId; > see comment in which the table was added, we should have 3 members here - i answered in the class Line 13: private String vmName; Line 14: private String entityType; Line 15: private String originalTemplateName; Line 16: private int origin; http://gerrit.ovirt.org/#/c/26480/11/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 5: Line 6: import org.ovirt.engine.core.common.businessentities.OvfEntityData; Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public interface UnregisteredOVFDataDAO extends DAO { > please change to extends GenericDao<OvfEntityData, Guid>, methods name will There is no need to use compensation for unregisteredOVF, so I don't see the need to use GenericDao here, also the key being used for unregistered OVF is generated from two columns (vm id and storage domain id). and GenericDAO only support one column. Line 10: Line 11: /** Line 12: * Retrieves the entity with the given VM id. Line 13: * Line 14: * @param id Line 15: * The VM Id. Line 16: * @return The entity instance, or <code>null</code> if not found. Line 17: */ Line 18: public OvfEntityData getByVmId(Guid vmId); > your key is the vm id and the storage domain id, which means that you might I will add the storage domain id Line 19: Line 20: /** Line 21: * Retrieves all the entities of type OvfEntityData related to the storage Domain Id. Line 22: * Line 25: * @param entityType Line 26: * The entity type (VM/Template) Line 27: * @return List of all the OvfEntityData related to the storage Domain Id, or an empty list if none is found. Line 28: */ Line 29: public List<OvfEntityData> getAllForStorageDomain(Guid storageDomainId, String entityType); > /s/getAllForStorageDomain/getAllForStorageDomainByEntityType done Line 30: Line 31: /** Line 32: * Insert new entity to the unregistered table. Line 33: */ Line 50: * Line 51: * @param vmId Line 52: * @param storageDomainId Line 53: */ Line 54: public void removeEntity(Guid vmId, Guid storageDomainId); > /s/vmId/entityId done http://gerrit.ovirt.org/#/c/26480/11/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/UnregisteredOVFDataDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/UnregisteredOVFDataDAOTest.java: Line 28: Line 29: @Test Line 30: public void testGetVMsForStorageDomain() { Line 31: dao = dbFacade.getUnregisteredOVFDataDao(); Line 32: List<OvfEntityData> ovfEntityDataList = dao.getAllForStorageDomain(FixturesTool.STORAGE_DOAMIN_NFS2_1, "VM"); > please use the appropiate constant instead of "VM" There is not existing constant Line 33: assertTrue("a VM should be fetched for the specified storage domain", !ovfEntityDataList.isEmpty()); Line 34: } Line 35: Line 36: @Test Line 36: @Test Line 37: public void testGetTempaltesForStorageDomain() { Line 38: dao = dbFacade.getUnregisteredOVFDataDao(); Line 39: List<OvfEntityData> ovfEntityDataList = Line 40: dao.getAllForStorageDomain(FixturesTool.STORAGE_DOAMIN_NFS2_1, "Template"); > please use the appropiate constant instead of "Template" There is no existing constant Line 41: assertTrue("a Tempalte should not be fetched for the specified storage domain", ovfEntityDataList.isEmpty()); Line 42: } Line 43: Line 44: @Test Line 58: Line 59: @Test Line 60: public void testInsertTemplateToUnregisteredEntity() { Line 61: dao = dbFacade.getUnregisteredOVFDataDao(); Line 62: final String OVF_EXTRA_DATA = "<ovf> Some extra OVF data </ovf>"; > please change the name to java conventions, It is not ovfData only ovfExtraData, change it to be with java convention Line 63: dao.insertOVFDataForEntities(FixturesTool.VM_TEMPLATE_RHEL5, "Stam", "TEMPLATE", null, 1, 1, 1, 1, null, 1, "3.4", FixturesTool.STORAGE_DOAMIN_NFS2_1, OVF_EXTRA_DATA); Line 64: OvfEntityData ovfEntityData = dao.getByVmId(FixturesTool.VM_TEMPLATE_RHEL5); Line 65: assertNotNull(ovfEntityData); Line 66: assertTrue("The entity type should be template", ovfEntityData.getEntityType().equals("TEMPLATE")); http://gerrit.ovirt.org/#/c/26480/11/packaging/dbscripts/unregistered_OVF_data_sp.sql File packaging/dbscripts/unregistered_OVF_data_sp.sql: Line 27: Create or replace FUNCTION RemoveEntityFromUnregistered(v_vm_guid UUID, v_storage_domain_id UUID) Line 28: RETURNS VOID Line 29: AS $procedure$ Line 30: DECLARE Line 31: v_val UUID; > unneeded removed Line 32: BEGIN Line 33: DELETE FROM unregistered_ovf_of_entities Line 34: WHERE vm_guid = v_vm_guid Line 35: AND storage_domain_id = v_storage_domain_id; Line 36: END; $procedure$ Line 37: LANGUAGE plpgsql; Line 38: Line 39: Line 40: Create or replace FUNCTION GetAllOVFEntitiesForStorageDomain(v_storage_domain_id UUID, v_entity_type VARCHAR(20)) > /s/GetAllOVFEntitiesForStorageDomain/GetAllOVFEntitiesForStorageDomainByEnt done Line 41: RETURNS SETOF unregistered_ovf_of_entities STABLE Line 42: AS $procedure$ Line 43: BEGIN Line 44: RETURN QUERY SELECT * http://gerrit.ovirt.org/#/c/26480/11/packaging/dbscripts/upgrade/03_05_0410_unregistered_ovf_of_entities.sql File packaging/dbscripts/upgrade/03_05_0410_unregistered_ovf_of_entities.sql: Line 1: CREATE TABLE unregistered_ovf_of_entities Line 2: ( > this table should contain three columns- entity_id, entity_type, ovf_data a after talking f2f, we have decided to keep only vm_name, acrchitecture, compatiblity version, storage_domain_id and vm_id. All the rest will be generated from the OVF Line 3: vm_guid UUID, Line 4: vm_name character varying(255) NOT NULL, Line 5: entity_type character varying(32) NOT NULL, Line 6: original_template_name character varying(255), -- 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: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[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
