Yevgeny Zaspitsky has posted comments on this change. Change subject: core: Make DAOs injectable ......................................................................
Patch Set 4: Code-Review-1 (9 comments) https://gerrit.ovirt.org/#/c/38505/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/BllCDIAdapter.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/BllCDIAdapter.java: Please remove this class. Once you rebase onto the updated master you'll see that after you change it's empty. Line 1: package org.ovirt.engine.core.bll.utils; Line 2: Line 3: import javax.enterprise.context.ApplicationScoped; Line 4: import javax.enterprise.inject.Produces; https://gerrit.ovirt.org/#/c/38505/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeSnapshotEntity.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeSnapshotEntity.java: Line 15: GlusterVolumeSnapshotEntity How that class could be a singleton? https://gerrit.ovirt.org/#/c/38505/4/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 274: Class<T> daoType = (Class<T>) mapEntityToDao.get(entityClass); Line 275: return getDao(daoType); Line 276: } Line 277: Line 278: public void initBaseDAO(BaseDAODbFacade dao) { 1. a managed beans should be initialized by the container. once we'll have the DAOs' dependencies (template and dialect) as singleton beans they could be injected into DAO's instead of being set programmatically and then this method will be redundant. BTW, I do not see a good reason for having DbFacade as a DAO dependency. 2. That is kind of circular dependy between DAOs and dbFacade. Line 279: dao.setTemplate(jdbcTemplate); Line 280: dao.setDialect(dbEngineDialect); Line 281: dao.setDbFacade(this); Line 282: } Line 275: return getDao(daoType); Line 276: } Line 277: Line 278: public void initBaseDAO(BaseDAODbFacade dao) { Line 279: dao.setTemplate(jdbcTemplate); Can't we have jdbcTemplate as a singleton bean being injected into all DAOs by DI? Line 280: dao.setDialect(dbEngineDialect); Line 281: dao.setDbFacade(this); Line 282: } Line 283: Line 276: } Line 277: Line 278: public void initBaseDAO(BaseDAODbFacade dao) { Line 279: dao.setTemplate(jdbcTemplate); Line 280: dao.setDialect(dbEngineDialect); same for the dialect Line 281: dao.setDbFacade(this); Line 282: } Line 283: Line 284: @SuppressWarnings("unchecked") Line 285: public 1. Why the method has to be public? It isn't being used out of the class. 2. Is that guaranteed that at time of this method called the context has all daos in it. If not we should find some CDI equivalent for Spring @DependsOn and use that in order to let CDI know that this bean is dependent on all DAOs. Line 288: initBaseDAO Why it's being called here? Isn't BaseDAODbFacade.init (with @PostConstruct) enough? https://gerrit.ovirt.org/#/c/38505/4/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BookmarkDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BookmarkDAOTest.java: should this be part of this patch? Line 1: package org.ovirt.engine.core.dao; Line 2: Line 3: import static org.junit.Assert.assertEquals; Line 4: import static org.junit.Assert.assertNotEquals; https://gerrit.ovirt.org/#/c/38505/4/backend/manager/modules/vdsbroker/pom.xml File backend/manager/modules/vdsbroker/pom.xml: Line 106: <version>5.0.0.GA</version> Line 107: <scope>test</scope> Line 108: </dependency> Line 109: --> Line 110: <dependency> what is this for? Line 111: <groupId>org.scala-lang</groupId> Line 112: <artifactId>scala-library</artifactId> Line 113: <version>2.10.4</version> Line 114: </dependency> -- To view, visit https://gerrit.ovirt.org/38505 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic426126d690ab6dedb49ef6b67deeba67661d031 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
