Vojtech Szocs has posted comments on this change.
Change subject: core: [CDI] Introduce CDI for Commands dependencies
......................................................................
Patch Set 14: Code-Review+1
Nice patch!
Some comments:
* I assume that META-INF/beans.xml activates CDI scanner to process all classes
in bll JAR at deployment time
* CommandsFactory contains only static methods, you can consider making it CDI
managed bean by using @ApplicationScoped
* Injector could be CDI managed bean just like CommandsFactory, it could inject
BeanManager from JNDI by using @Resource(lookup="java:comp/BeanManager")
* instead of JNDI lookup, you can consider doing CDI.current().getBeanManager()
You could also move things a bit further by eliminating use of
constructor.newInstance when creating commands:
* CDI injector (BeanManager) should be the one responsible for instantiating
commands
* instead of CommandsFactory.createCommand you would simply ask CDI injector to
create non-singleton (one-time-use) instance of given command
* since command params is dynamic input rather than regular dependency, it
wouldn't make sense for command params to be CDI managed beans themselves
* example to illustrate ideas mentioned above:
// create non-singleton (one-time-use) instance of given command
// 1. resolve command Class
// 2. resolve Bean from Class
// 3. instruct CDI injector (CreationalContext?) to satisfy @Params with given
parameters instance
// 4. ask CDI injector to instantiate command
// @Params indicates dynamic (non-managed) input to use during instantiation
public AddWatchdogCommand(@Params WatchdogParameters parameters) {
...
}
Reference: https://docs.jboss.org/weld/reference/latest/en-US/html/extend.html
("The InjectionTarget interface")
"However, if the framework requires use of a constructor with a special
signature, the framework will need to instantiate the object itself, and so
only method and field injection will be supported."
The above quote would be the use-case for instantiating command classes via CDI
(need to pass custom parameters argument).
--
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: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Asaf Shakarchi <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[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: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: mooli tayer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
