Re: [cdi-dev] Feedback - CDI bootstrap API (CDI-26)

My point is that from the application perspective, the user obtains one container handle for eventual shutdown (CDIContainer) and then looks up a different container handle (CDI) that they can use for real work (lookup / event dispatch / etc.) It would be cleaner if the container gave away a single handle that can do all of that. On 02/26/2015 05:42 PM, Romain Manni-Bucau wrote: Not sure I get how a CDI instance can help. But container.getBeanManager() sounds nice is not a shortcut for CDI.current().getBm() otherwise it looks like duplication to me. Can we make container not contextual - dont think so? If so it makes sense otherwise I fear it doesnt add much. Le 26 févr. 2015 16:19, "Jozef Hartinger" <jharting(a)redhat.com&gt; a écrit : > > I like the initialize + close() combination and the try-with-resources > usage. > What looks weird to me is that at line one you obtain a container handle: > > try (CDIContainer container = CDIContainer.newCDIContai... > CDI.current().getBeanManager() ... > > and then at line two you call a static method to perform a container > lookup :-/ > > An API that allows you to use the container handle you already got is way > better IMO, e.g.: > > try (CDIContainer container = CDIContainer.newCDIContai... > container.getBeanManager() > > If CDIContainer.newCDIContainer() returns an CDI instance or its subclass, > we get this easily. > > On 02/26/2015 08:58 AM, Romain Manni-Bucau wrote: >> >> Hi guys >> >> why note keeping it simple? >> >> try (CDIContainer container = CDIContainer.newCDIContainer(/* optional >> map to configure vendor features */)) { >> CDI.current().getBeanManager().... >> } >> >> Not sure the point having initialize() + having shutdown = close >> really makes the API more fluent and modern IMO. >> >> Also to be fully SE I guess provider() method would be needed even if >> optional (SPI usage by default): >> >> try (CDIContainer container = >> >> CDIContainer.provider("org.jboss.weld.WeldCdiContainerProvider").newCDIContainer()) >> { >> CDI.current().getBeanManager().... >> } >> >> Finally I think having a kind of getInstance shortcut could be a plus for >> SE: >> >> try (CDIContainer container = CDIContainer.newCDIContainer()) { >> container.newInstance(MyAppRunner.class /* optional qualifiers */ >> ).run(args); >> } >> >> Using container to get an instance would create the instance and bind >> it to the container lifecycle (mainly for predestroy) avoiding this >> boilerplate code in all main which will surely only be used to launch >> a soft. >> >> wdyt? >> >> >> >> Romain Manni-Bucau >> @rmannibucau >> http://www.tomitribe.com >> http://rmannibucau.wordpress.com >> https://github.com/rmannibucau >> >> >> 2015-02-26 8:32 GMT+01:00 Jozef Hartinger <jharting(a)redhat.com&gt;: >>> >>> Comments inline >>> >>> On 02/25/2015 05:53 PM, John D. Ament wrote: >>> >>> Sorry Jozef, your email fell into the pits of google inbox's "smart >>> sorting" >>> features. >>> >>> On Thu, Feb 12, 2015 at 3:18 AM Jozef Hartinger <jharting(a)redhat.com&gt; >>> wrote: >>>> >>>> Hi John, comments inline: >>>> >>>> >>>> On 02/11/2015 06:02 PM, John D. Ament wrote: >>>> >>>> Jozef, >>>> >>>> Most of what you see there is taken from the original doc, since >>>> everyone >>>> seemed to be in agreement. I think the map is just a safeguard in case >>>> of >>>> additional boot options available in some implementations (e.g. I think >>>> OWB/OpenEJB have some options.. currently OpenEJB supports an embedded >>>> CDI >>>> boot mode). >>>> >>>> No, I am fine with the map. What I am questioning is the type of the >>>> map. >>>> Usually, data structures with a similar purpose use Strings as their >>>> keys. >>>> This applies to ServletContext attributes, InvocationContext data, >>>> Servlet >>>> request/session attributes and others. I am therefore wondering whether >>>> there is a usecase for the proposed unbound key signature or not. >>> >>> >>> I think that's more of a placeholder, I was assuming it would be >>> Map<String,Object> once we clarify everything. >>> >>>> >>>> >>>> We spoke a few times about BeanManager vs CDI. BeanManager was >>>> preferable >>>> since there's no easy way to get the the instance, CDI is easier to get >>>> and >>>> more aligned with how you would get it. Usually people expect the >>>> BeanManager to be injected or available via JNDI, neither would be the >>>> case >>>> here. >>>> >>>> If CDI 2.0 targets Java SE then this container initialization API will >>>> become something that ordinary application developers use to start/stop >>>> CDI >>>> in their applications. It therefore cannot be considered an SPI but >>>> instead >>>> should be something easy to use. On the other hand, BeanManager is >>>> definitely an SPI. It is used in extension, frameworks and generally >>>> for >>>> integration. Not much by applications directly. Therefore, I don't see >>>> how >>>> the container bootstrap API and BeanManager fit together. IMO the >>>> bootstrap >>>> API should expose something that makes common tasks (obtaining a >>>> contextual >>>> reference and firing and event) easy, which the CDI class does. >>>> >>>> Plus do not forget that BeanManager can be obtained easily using >>>> CDI.getBeanManager(). >>> >>> >>> I'm not disagreeing. There's a few things I'd consider: >>> >>> - Is this mostly for new apps or existing? If existing, it's probably >>> using >>> some internal API, if new it can use whatever API we give. >>> - I don't want to return void, we should give some kind of reference >>> into >>> the container when we're done booting. >>> >>> Agreed, we should not be returning void. >>> >>> - CDI is a one step retrievable reference, where as BeanManager is a two >>> step reference. With that said, BeanManager makes more sense to return >>> here. Another thought could be we invent some new class that has both, >>> but >>> that's really redundant. >>> >>> Why do you think BeanManager makes more sense here? Especially given the >>> assumption that application code is going to call this init/shutdown >>> API, I >>> don't see BeanManager as making more sense. >>> >>> >>>> >>>> >>>> Yes, this is the container start API. Sounds like you have some good >>>> ideas for things like XML configuration or programmatic configuration, >>>> both >>>> of which are being tracked under separate tickets. One idea might be >>>> for an >>>> optional param in the map to control packages to scan/ignore, in that >>>> map. >>>> >>>> I am wondering whether this configuration should be something optional >>>> built on top of the bootstrap API or whether we should consider making >>>> it >>>> mandatory. Either way, we cannot add the bootstrap API to the spec >>>> without >>>> explicitly defining how it behaves. My implicit assumption of the >>>> proposal >>>> is that the container is supposed to scan the entire classpath for >>>> explicit >>>> or implicit bean archives (including e.g. rt.jar), discover beans, fire >>>> extensions, etc. This worries me as this default behavior is far from >>>> being >>>> lightweight, which CDI for Java SE initially aimed to be. >>> >>> >>> Yes, the spec must be updated to reflect the behavior of SE mode. I >>> plan to >>> get that completely into the google doc before opening any spec changes >>> in a >>> PR. >>> >>>> >>>> >>>> We didn't want to over load the CDI interface. It already does a lot. >>>> This is really SPI code, CDI even though it's in the spi package is >>>> used in >>>> a lot of application code. >>>> >>>> I would personally prefer to have it all in one place. Having >>>> CDIContainer, CDIContainerLoader, CDI and CDIProvider makes it more >>>> difficult to know when to use what. >>> >>> >>> The problem is that most CDI (the interface) operations are against a >>> running container. I think we spoke about leveraging CDIProvider at one >>> point (in fact, I mistakenly called CDIContainer CDIProvider not even >>> realizing it was there). I doubt that most app developers use it >>> currently, >>> there's not even a way to get a reference to it that I'm aware of. It's >>> used by the implementor only. >>> >>> I don't think there's a conflict. CDI class would still only provide >>> methods >>> to be run against a running container. The difference is that there >>> would be >>> additional static methods to get this running container (CDI class) to >>> you >>> by starting the container. >>> >>> Either way, I agree that reusing CDIProvider is a must. There is no >>> reason >>> to define a new class for the same purpose. >>> >>> >>> I expect that my changes in the CDI spec around this will state, along >>> the >>> lines of: >>> >>> To retrieve a CDIContainer to launch, do this: >>> >>> CDIContainer container = CDIContainerLocator.getCDIContainer(); >>> container.initialize(); >>> ... do work >>> >>> Once you want to shutdown the container, do this: >>> >>> container.shutdown(); >>> >>> (we may want to consider implementing AutoCloseable, an oversight on my >>> part) >>> >>> and then later on >>> >>> - What happens if I call CDIContainerLocator in an app server >>> >>> - It throws an IllegalStateException. >>> >>> - The container provides no beans of type CDIContainer, it is managed >>> outside of the CDI container. >>> >>> >>>> >>>> >>>> John >>>> >>>> On Wed Feb 11 2015 at 4:21:50 AM Jozef Hartinger <jharting(a)redhat.com&gt; >>>> wrote: >>>>> >>>>> Hi John, some thoughts: >>>>> >>>>> - instead of using BeanManager it makes more sense to me to return a >>>>> CDI >>>>> instance, which is a more user-friendly API (and it also exposes >>>>> access to >>>>> BeanManager) >>>>> - is there a usecase for arbitrary keys of the "params" map or is >>>>> Map<String, ?> sufficient? >>>>> - if we could move the shutdown() method from CDIContainer to the >>>>> actual >>>>> container handle that we obtain from initialize(), that would look >>>>> more >>>>> object-oriented >>>>> - what exactly is initialize() supposed to do? Is it supposed to start >>>>> scanning the entire classpath for CDI beans? That could be a problem >>>>> especially with spring-boot-like fat jars. I think we need an API to >>>>> tell >>>>> the container which classes / packages to consider. Something like >>>>> Guice's >>>>> binding API perhaps? >>>>> >>>>> - the proposal makes me wonder whether retrofitting this functionality >>>>> to >>>>> the CDI class wouldn't be a better option. It could look like: >>>>> >>>>> CDI container = CDI.initialize(); >>>>> container.select(Foo.class).get(); >>>>> container.shutdown(); >>>>> >>>>> compare it to: >>>>> >>>>> CDIContainer container = CDIContainerLoader. getCDIContainer(); >>>>> BeanManager manager = container.initialize(); >>>>> manager.getBeans(...); >>>>> container.shutdown(manager); >>>>> >>>>> >>>>> On 02/10/2015 06:58 PM, John D. Ament wrote: >>>>> >>>>> All, >>>>> >>>>> I have the updated API here, and wanted to solicit any final feedback >>>>> before updating the google doc and spec pages. >>>>> >>>>> >>>>> >>>>> https://github.com/johnament/cdi/commit/2c362161e18dd521f8e83c27151ddad46... >>>>> >>>>> Let me know your thoughts. >>>>> >>>>> Thanks, >>>>> >>>>> John >>>>> >>>>> >>>>> _______________________________________________ >>>>> cdi-dev mailing list >>>>> cdi-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/cdi-dev >>>>> >>>>> Note that for all code provided on this list, the provider licenses >>>>> the >>>>> code under the Apache License, Version 2 >>>>> (http://www.apache.org/licenses/LICENSE-2.0.html). For all other ideas >>>>> provided on this list, the provider waives all patent and other >>>>> intellectual >>>>> property rights inherent in such information. >>>>> >>>>> >>> >>> _______________________________________________ >>> cdi-dev mailing list >>> cdi-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/cdi-dev >>> >>> Note that for all code provided on this list, the provider licenses the >>> code >>> under the Apache License, Version 2 >>> (http://www.apache.org/licenses/LICENSE-2.0.html). For all other ideas >>> provided on this list, the provider waives all patent and other >>> intellectual >>> property rights inherent in such information. > >