Juan Hernandez has posted comments on this change. Change subject: core: WIP: Adding JPA to ovirt ......................................................................
Patch Set 1: (9 comments) Take into account that using Hibernate also has the side effect of delaying actual updates till the transaction is committed. This may have adverse effects combined with the stored procedures, as in the same transaction procedures (invoked outside of the control of Hibernate) may see stale data. You may also want to consider using external XML files for metadata instead of annotations. I don't think they are better, but it may be easier to re-introduce JPA gradually if we don't have to modify/review the entities. I don't know if you are aware that there was a previous attempt to introduce JPA. It failed and was completely removed: http://gerrit.ovirt.org/11590 You may want to dig and find why it failed. .................................................... File backend/manager/modules/common/pom.xml Line 50: <dependency> Line 51: <groupId>org.hibernate</groupId> Line 52: <artifactId>hibernate-entitymanager</artifactId> Line 53: <version>4.3.0.Final</version> Line 54: </dependency> You should add here explicitly the dependency for the artifact containing the javax.annotations package. Even if the hibernate-entitymanager artifact brings the dependency implicitly it is good practice to explicitly add dependencies for anything that you are importing in the Java code. Line 55: Line 56: </dependencies> Line 57: Line 58: <build> .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java Line 100: @Column(name = "trusted_service") Line 101: private boolean trustedService; Line 102: Line 103: @Column(name = "cluster_policy_id") Line 104: @Type(type = "org.ovirt.engine.core.dao.GuidMapper") You are going to have to repeat this a lot, I guess. Maybe worth an @TypeDef in a package-info.java file. Line 105: private Guid clusterPolicyId; Line 106: Line 107: @Transient Line 108: private String clusterPolicyName; .................................................... File backend/manager/modules/dal/pom.xml Line 87: <dependency> Line 88: <groupId>org.hibernate</groupId> Line 89: <artifactId>hibernate-entitymanager</artifactId> Line 90: <version>4.3.0.Final</version> Line 91: </dependency> You should add here the dependency on the artifact containing the javax.persistence classes. Line 92: </dependencies> Line 93: <build> Line 94: <filters> Line 95: <filter>src/test/filters/${engine.db}.properties</filter> .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/GuidMapper.java Line 33: Line 34: if (val instanceof Guid) Line 35: return new Guid(((Guid) val).getUuid()); Line 36: Line 37: throw new UnsupportedOperationException("Can't convert " + val.getClass() + " to GUID"); Shouldn't this be HibernateException? Line 38: } Line 39: Line 40: @Override Line 41: public Serializable disassemble(Object val) throws HibernateException { Line 45: Line 46: if (val instanceof UUID) Line 47: return new Guid((UUID) val); Line 48: Line 49: throw new UnsupportedOperationException("Can't convert " + val.getClass() + " to GUID"); Same as above. Line 50: } Line 51: Line 52: @Override Line 53: public boolean equals(Object x, Object y) throws HibernateException { .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsGroupDAODbFacadeImpl.java Line 26: * <code>VdsGroupDAODbFacadeImpl</code> provides an implementation of {@link VdsGroupDAO} that uses code previously Line 27: * found in {@link org.ovirt.engine.core.dal.dbbroker.DbFacade}. Line 28: * Line 29: */ Line 30: public class VdsGroupDAODbFacadeImpl extends HibernateFacade implements VdsGroupDAO { In the past, when we had hibernate we used to have separate classes *DbFacadeImpl and *HibernateImpl. That allows switching the technology changing the engine-dao.properties file. Did you consider doing that? Line 31: @Override Line 32: public VDSGroup get(Guid id) { Line 33: return getEntityManager().find(VDSGroup.class, id); Line 34: } Line 40: CriteriaQuery<VDSGroup> query = cb.createQuery(VDSGroup.class); Line 41: Root<VDSGroup> group = query.from(VDSGroup.class); Line 42: query.where(cb.and(cb.equal(group.get("id"), id), Line 43: cb.and(cb.equal(group.get("user_id"), userID), Line 44: cb.equal(group.get("is_filtered"), isFiltered)))); JPQL or (HQL) seems a better and less verbose alternative for this kind of query. Why do you prefer the criteria API? Line 45: Line 46: return em.createQuery(query).getSingleResult(); Line 47: } Line 48: .................................................... File backend/manager/modules/dal/src/main/resources/META-INF/persistence.xml Line 1: <persistence xmlns="http://java.sun.com/xml/ns/persistence"; 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"> Where does this persistence unit gets the connections from? Don't you need something like this: <jta-data-source>java:/ENGINEDataSource</jta-data-source> I would suggest to add that even if that is the only data source deployed to the application server, as other applications (reports, for example) may otherwise deploy other data sources and invalidate the configuration. Line 6: <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider> Line 7: <class>org.ovirt.engine.core.common.businessentities.VDSGroup</class> Line 8: <properties> Line 9: <property name="hibernate.dialect" value="org.hibernate.dialect.PostgreSQLDialect" /> .................................................... File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VdsGroupDAOTest.java Line 50: is = BaseDAOTestCase.class.getResourceAsStream("/test-database.properties"); Line 51: try { Line 52: properties.load(is); Line 53: } catch (Exception e) { Line 54: e.printStackTrace(); Why are you catching this exception? Shouldn't this failure just fail the test? Line 55: } Line 56: Line 57: Map props = new HashMap(); Line 58: props.put("javax.persistence.jdbc.driver", properties.getProperty("database.driver")); -- 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: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Tal Nisan <[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
