Roy Golan has posted comments on this change. Change subject: core: CDI - Replace EJBs with CDI Singletons ......................................................................
Patch Set 5: (12 comments) http://gerrit.ovirt.org/#/c/19887/5/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 253: /** Line 254: * just convenience so we don't refactor old code Line 255: */ Line 256: public static DbFacade getInstance() { Line 257: return instance; > Throw an UnsupportedOperationException here. At least as a TODO: this is kept for code path which uses this witout injectio yet so have to keep it I can annotate it @Deprecated. Line 258: } Line 259: Line 260: private CustomMapSqlParameterSource getCustomMapSqlParameterSource() { Line 261: return new CustomMapSqlParameterSource(dbEngineDialect); http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/TimeoutBase.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/TimeoutBase.java: Line 12: CacheManager > protected? Done http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDAOTestCase.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDAOTestCase.java: Line 62: dataSource = createDataSource(); Line 63: ejbRule.mockResource(ContainerManagedResourceType.DATA_SOURCE, dataSource); Line 64: Line 65: dataset = initDataSet(); Line 66: dbFacade = new DbFacade(); > Really? Why not inject dbFacade? one change at a time. check out my Arquilian based test to have members injected. btw - can SpringJUnit4ClassRunner inject members? Line 67: dbFacade.setDbEngineDialect(DbFacadeLocator.loadDbEngineDialect()); Line 68: // load data from fixtures to DB Line 69: DatabaseOperation.CLEAN_INSERT.execute(getConnection(), dataset); Line 70: } http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/BackendApplication.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/BackendApplication.java: Line 82: private ValidatorLocator validatorLocator; Line 83: private SessionHelper sessionHelper; Line 84: Line 85: // The reference to the backend bean: Line 86: @javax.inject.Inject > Can't you import the lass and use it without the package name? Done Line 87: private BackendLocal backend; Line 88: Line 89: // The set of singletons: Line 90: private Set<Object> singletons = new HashSet<Object>(); Line 114: Context initial = new InitialContext(); Line 115: BeanManager beanManager = (BeanManager) initial.lookup("java:comp/BeanManager"); Line 116: Bean backendBean = beanManager.getBeans(BackendLocal.class).iterator().next(); Line 117: CreationalContext backendCc = beanManager.createCreationalContext(backendBean); Line 118: backend = (BackendLocal) beanManager.getReference(backendBean, BackendLocal.class, backendCc); > Indent this correctly. Done Line 119: } Line 120: catch (Exception exception) { Line 121: logger.error("Can't find reference to backend bean.", exception); Line 122: throw exception; http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/SchedulerUtilQuartzImpl.java File backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/SchedulerUtilQuartzImpl.java: Line 44: Line 45: @Inject Line 46: private Log log; Line 47: private Scheduler sched; Line 48: private static volatile SchedulerUtilQuartzImpl instance; > If the class is a singleton, why does this needs to be static? to have the legacy static getInstance() call to work. in time we will cleanup all of this getInstance() calls I'll annotate it @Deprecated as well Line 49: Line 50: private final AtomicLong sequenceNumber = new AtomicLong(Long.MIN_VALUE); Line 51: Line 52: /** http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/utils/pom.xml File backend/manager/modules/utils/pom.xml: Line 166: <dependency> Line 167: <groupId>javax.enterprise</groupId> Line 168: <artifactId>cdi-api</artifactId> Line 169: <scope>provided</scope> Line 170: </dependency> > This dependency is duplicated. Done Line 171: Line 172: Line 173: </dependencies> Line 174: http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ejb/EJBUtilsStrategy.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ejb/EJBUtilsStrategy.java: Line 33: protected EJBUtilsStrategy() { Line 34: // Adds JNDI resources, Line 35: addJNDIResources(); Line 36: // Adds JNDI beans - the implementation is in base classes Line 37: addJNDIBeans(); > Can't this class be removed altogether? it should but there is Test dependency on MockEjbStrategyRule I should resolve first. Line 38: Line 39: } Line 40: Line 41: protected abstract void addJNDIBeans(); http://gerrit.ovirt.org/#/c/19887/5/backend/manager/modules/welcome/src/main/java/org/ovirt/engine/core/WelcomeServlet.java File backend/manager/modules/welcome/src/main/java/org/ovirt/engine/core/WelcomeServlet.java: Line 45: /** Line 46: * Back-end bean for database access. Line 47: */ Line 48: @Inject Line 49: private BackendLocal backend; > You need to keep the "transient", it was added to fix a findbugs (or coveri Done Line 50: Line 51: /** Line 52: * The branding manager. Line 53: */ http://gerrit.ovirt.org/#/c/19887/5/ear/src/main/resources/META-INF/jboss-deployment-structure.xml File ear/src/main/resources/META-INF/jboss-deployment-structure.xml: Line 7: <module name="javax.inject.api"/> Line 8: <module name="javax.interceptor.api"/> Line 9: <module name="org.ovirt.engine.core.common" export="true" meta-inf="import"/> Line 10: <module name="org.ovirt.engine.core.utils" export="true" meta-inf="import"/> Line 11: <module name="org.ovirt.engine.core.dal" export="true" meta-inf="import"/> > I think you don't need the "export" here, as that means that you export the your talking about everything or just dal? without it the container wasn't aware of those beans Line 12: </dependencies> Line 13: </deployment> http://gerrit.ovirt.org/#/c/19887/5/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GenericApiGWTServiceImpl.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GenericApiGWTServiceImpl.java: Line 192: "browser. To solve the issue the user needs " + //$NON-NLS-1$ Line 193: "to close the browser and open it again, so " + //$NON-NLS-1$ Line 194: "that the application is reloaded.", //$NON-NLS-1$ Line 195: error Line 196: ); > It is probably better to avoid changes like this, as they make the patch la that's an accidental IDE formatting. removing this offcourse Line 197: } Line 198: Line 199: // Now that we replaced the message let GWT do what it uses to do: Line 200: super.doUnexpectedFailure(error); http://gerrit.ovirt.org/#/c/19887/5/pom.xml File pom.xml: Line 302: <artifactId>snakeyaml</artifactId> Line 303: <version>${snakeyaml.version}</version> Line 304: </dependency> Line 305: <dependency> Line 306: <<<<<<< HEAD > Please take care of this merge conflict. Done Line 307: <groupId>org.slf4j</groupId> Line 308: <artifactId>slf4j-api</artifactId> Line 309: <version>${slf4j.version}</version> Line 310: </dependency> -- To view, visit http://gerrit.ovirt.org/19887 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I126fa3f4240e81814ec9e902cb2e93ce364589ff Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: mooli tayer <[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
