Liran Zelkha has posted comments on this change.

Change subject: core: Make DAOs injectable
......................................................................


Patch Set 4:

(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.
Done
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?
Done


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.
True. I plan a future patch to fix DbFacade, Dialect, Template and 
DbFacadeLocator. But it's out side the scope of this patch.
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
True. As above answer.
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
True. As above answer.
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.
1. Fixed.
2. Since DbFacade has an injectible list of all the DAOs - the list is 
initialized.  (See the @Any annotation).


Line 288: initBaseDAO
> Why it's being called here? Isn't BaseDAODbFacade.init (with @PostConstruct
We use CDI in runtime (which supports the @Any annotation and injects all the 
beans for us) and Spring in the Unit Tests. Spring doesn't support the @Any 
annotation, and the list of DAOs is injected by the CDIIntegration class. 
This will be fixed automatically once the said patch will be written.


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?
Done
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?
Done
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 <[email protected]>
Gerrit-Reviewer: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Yevgeny Zaspitsky <[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

Reply via email to