Juan Hernandez has posted comments on this change.

Change subject: core: CDI - Replace EJBs with CDI Singletons
......................................................................


Patch Set 5:

(8 comments)

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?
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.
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/searchbackend/src/main/resources/rebel.xml
File backend/manager/modules/searchbackend/src/main/resources/rebel.xml:

Line 5:                 <dir 
name="/home/rgolan/src/git/ovirt-engine/backend/manager/modules/searchbackend/target/classes">
Line 6:                 </dir>
Line 7:         </classpath>
Line 8: 
Line 9: </application>
Do you really want to include this file in the patch?


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.
Line 171: 
Line 172: 
Line 173:   </dependencies>
Line 174: 


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 coverity, 
not sure) error.
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 
dependencies to other modules, and no module depends on the ear.
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 larger 
and harder to review.
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.
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

Reply via email to