Yair Zaslavsky has posted comments on this change. Change subject: core: WIP: Adding JPA to ovirt ......................................................................
Patch Set 15: (12 comments) http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/common/pom.xml File backend/manager/modules/common/pom.xml: Line 55: <groupId>org.hibernate</groupId> Line 56: <artifactId>hibernate-entitymanager</artifactId> Line 57: <version>4.3.0.Final</version> Line 58: </dependency> Line 59: please remove whitespace. Line 60: <dependency> Line 61: <groupId>javax.annotation</groupId> Line 62: <artifactId>javax.annotation-api</artifactId> Line 63: <version>1.2</version> Line 61: <groupId>javax.annotation</groupId> Line 62: <artifactId>javax.annotation-api</artifactId> Line 63: <version>1.2</version> Line 64: </dependency> Line 65: No need for some jpa dependency? How do i know that this is JPA 2.0 and not JPA 1.0? Line 66: </dependencies> Line 67: Line 68: <build> Line 69: <plugins> http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Permissions.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Permissions.java: Line 24: @Column(name = "object_id") Line 25: private Guid objectId; Line 26: @Transient Line 27: private String objectName; Line 28: @Transient Why Transient on these ones? Line 29: private VdcObjectType objectType; Line 30: @Transient Line 31: private String roleName; Line 32: @Transient http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Securityid.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Securityid.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import java.io.Serializable; Line 4: Line 5: import javax.persistence.Column; is this an embaddable class? Is there special annotation we need for that if so? Line 6: Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public class Securityid implements Serializable { Line 5: import javax.persistence.Column; Line 6: Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public class Securityid implements Serializable { s/Securityid/SecurityId. Please elaborate on why you introduced this class? Line 10: Line 11: private static final long serialVersionUID = 7893248654789260759L; Line 12: Line 13: @Column(name = "entity_id") http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java: Line 66: @Column(name = "storage_pool_id", insertable = false, updatable = false) Line 67: @Type(type = "org.ovirt.engine.core.dao.GuidMapper") Line 68: private Guid storagePoolId; Line 69: Line 70: @ManyToOne(fetch = FetchType.EAGER, optional = false, cascade = CascadeType.ALL) Why EAGER? I'm quite rusty with JPA and Hibernate, but why not use left join fetch in the relevant JPA-QL queries and have this FetchType.LAZY? Line 71: @JoinColumn(name = "storage_pool_id") Line 72: private StoragePool storagePool; Line 73: Line 74: @Size(max = BusinessEntitiesDefinitions.DATACENTER_NAME_SIZE) http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroupUsers.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroupUsers.java: Line 11: @Table(name = "user_vds_groups_permissions_view") Line 12: public class VDSGroupUsers { Line 13: Line 14: @EmbeddedId Line 15: private Securityid id; s/Securityid/SecurityId Line 16: Line 17: @Column(name = "entity_id", insertable = false, updatable = false) Line 18: private Guid entityId; Line 19: http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/dal/pom.xml File backend/manager/modules/dal/pom.xml: Line 86: </dependency> Line 87: <dependency> Line 88: <groupId>javax.persistence</groupId> Line 89: <artifactId>persistence-api</artifactId> Line 90: <version>1.0.2</version> if this is JPA, why not JPA 2? Line 91: </dependency> Line 92: </dependencies> Line 93: <build> Line 94: <filters> http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/HashMapMapper.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/HashMapMapper.java: Line 12: import org.hibernate.engine.spi.SessionImplementor; Line 13: import org.hibernate.usertype.UserType; Line 14: import org.ovirt.engine.core.utils.SerializationFactory; Line 15: Line 16: public class HashMapMapper implements UserType { Why is this needed? I mean, I know we have has map fields, but doesn't JPA support hash maps? Is it because the content of the hash map is kept in the same table, and not in a different table? See for example - http://stackoverflow.com/questions/3393649/storing-a-mapstring-string-using-jpa Line 17: Line 18: @Override Line 19: public Object assemble(Serializable cached, Object owner) throws HibernateException { Line 20: return cached; http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/dal/src/main/resources/META-INF/persistence.xml File backend/manager/modules/dal/src/main/resources/META-INF/persistence.xml: Line 2: xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; Line 3: xsi:schemaLocation="http://java.sun.com/xml/ns/persistence http://java.sun.com/xml/ns/persistence/persistence_2_0.xsd"; Line 4: version="2.0"> Line 5: <persistence-unit name="ovirt"> Line 6: <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider> whitespace + identation issue? Line 7: <class>org.ovirt.engine.core.common.businessentities.VDSGroup</class> Line 8: <class>org.ovirt.engine.core.common.businessentities.StoragePool</class> Line 9: <class>org.ovirt.engine.core.common.businessentities.Permissions</class> Line 10: <class>org.ovirt.engine.core.common.scheduling.ClusterPolicy</class> http://gerrit.ovirt.org/#/c/22806/15/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ClusterMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ClusterMapper.java: Line 206: if (model.isSetPolicy()) { Line 207: SchedulingPolicyType policyType = SchedulingPolicyType.fromValue(model.getPolicy()); Line 208: if (policyType != null) { Line 209: ClusterPolicy clusterPolicy = new ClusterPolicy(); Line 210: clusterPolicy.setName(policyType.name().toLowerCase()); Why is this needed? rebase issue? Line 211: clusterPolicy.setId(null); Line 212: entity.setClusterPolicy(clusterPolicy); Line 213: } Line 214: } http://gerrit.ovirt.org/#/c/22806/15/packaging/dbscripts/vds_groups_sp.sql File packaging/dbscripts/vds_groups_sp.sql: Line 63 Line 64 Line 65 Line 66 Line 67 I would suggest a different approach here - a. Keep all SP code intact. b. Have the ability to configure whether we would like to use jdbc-based or JPA-based DAO. thanks! -- To view, visit http://gerrit.ovirt.org/22806 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cd0bbf9f0913955cb3e1facfa9a4bdc1f1ab24d Gerrit-PatchSet: 15 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
