Yair Zaslavsky has posted comments on this change.

Change subject: WIP: working CDI solution for backend
......................................................................


Patch Set 6: (3 inline comments)

Why do we need an empty beans.xml file?

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java
Line 47:         try {
Line 48:             Constructor<CommandBase<? extends 
VdcActionParametersBase>> constructor =
Line 49:                     
findCommandConstructor(getCommandClass(action.name(), CommandSuffix), 
parameters.getClass());
Line 50: 
Line 51:             CommandBase<P> cmd = (CommandBase<P>) 
constructor.newInstance(new Object[] { parameters });
Please add a comment explaining why you still have to use cmd.inject and when 
it will be eliminated.
Line 52:             injector.inject(cmd);
Line 53:             return cmd;
Line 54:         }
Line 55: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Injector.java
Line 10:     private BeanManager manager;
Line 11: 
Line 12:     public Injector() {
Line 13:         try {
Line 14:             this.manager = (BeanManager) new 
InitialContext().lookup("java:comp/BeanManager");
Can we use here the EJBUtils strategy? we use it to hide the lookups where 
possible.
Just an idea...
Line 15:         } catch (NamingException e) {
Line 16:             // if the bean manager couldn't be fetched then we 
couldn't inject dependencies to non-bean objects
Line 17:             throw new RuntimeException("Couldn't locate a BeanManager 
instance due to " + e.getMessage()
Line 18:                     + " Actions will fail to perform due to runtime 
missing dependencies");


Line 12:     public Injector() {
Line 13:         try {
Line 14:             this.manager = (BeanManager) new 
InitialContext().lookup("java:comp/BeanManager");
Line 15:         } catch (NamingException e) {
Line 16:             // if the bean manager couldn't be fetched then we 
couldn't inject dependencies to non-bean objects
Please add log.debug for the stack trace.
Line 17:             throw new RuntimeException("Couldn't locate a BeanManager 
instance due to " + e.getMessage()
Line 18:                     + " Actions will fail to perform due to runtime 
missing dependencies");
Line 19:         }
Line 20:     }


-- 
To view, visit http://gerrit.ovirt.org/5575
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f604ff91847b698efe84a09f724ba0492a672c1
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Asaf Shakarchi <a...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to