Le 26 févr. 2015 16:19, "Jozef Hartinger" <jharting@redhat.com> 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@redhat.com>:

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@redhat.com> 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@redhat.com>
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/2c362161e18dd521f8e83c27151ddad467a1c01c

Let me know your thoughts.

Thanks,

John


_______________________________________________
cdi-dev mailing list
cdi-dev@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@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.