-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Architecture Board] Clean up SPI to provide implementations #43
Comments
I suggest a solution to consider for all specs:
This would have the following advantages:
|
However, @rmannibucau and I also suggest that this Java SE based SPI mechanism should only be used in MP specs that can't rely on CDI. All other specs should only support CDI-based SPI mechanism and expect a CDI bean that implements a specific interface. |
Using CDI should be as easy as |
basically agree. But be careful to not put every SPI class in the same pot.
|
You cant use CDI.select, this is insanely slow and at the end static utilities are worde than normal injections so no real need and respecting cdi programming model solves it smoothly and enhance the platform consistency. Only goods and no real need to spli api/spi in 2 jars anymore ;). edit: select was missing turning my sentence in sthg weird |
I assume s/cant/can/? |
@struberg "select" got eaten by the keyboard, the static lazy lookup i meant |
To expand on @struberg's second use case for SPIs, here's a concrete instance of that in MicroProfile Reactive Streams Operators. The API artifact provides a set of builders, these builders are concrete classes, they don't provide any real business logic, they just build graphs of streams (a graph being a set of stages, eg, map, filter, etc). These builders build reactive streams publishers/subscribers/etc, and they do that by creating a graph, and then passing that graph to the vendor implementation through the SPI, this is where the actual work is then done by an implementation to turn that graph that describes the stream into a stream that can be run. In this case, the SPI is a combination of two things, a simple interface that the implementations implement, plus the concrete classes that represent the graph built by the builder API. While the interface itself could be extracted into a separate artifact, the API depends on the concrete classes, and so even if they were extracted, the api artifact would have to depend on the SPI artifact and so it wouldn't make sense to extract it. One of the reasons that it's done this way is as @struberg says, to allow customisation. There are some very big differences in the way reactive streams can be implemented - particularly when it comes to execution contexts, such as how context gets propagated, as well as monitoring and tracing. We want end users to be able to plug in different implementations for different use cases. MicroProfile containers will provide a default implementation, but users can, on a case by case basis, use different implementations as they please. Sometimes also it may be necessary to customise the default implementation for a particular use case within an application, so this also allows for that. So while we don't necessarily want this SPI exposed to end users, we do want it exposed to the libraries they use. Of course Java/Java EE offers no way to differentiate between library code and end user code. |
If there is a mp reactive one day, it must be cdi integrated and all based on interfaces and implemented as cdi beans IMHO to 1. avoid the cost of the provider (if you say you cache it then it doesnt work in most cases in java) and 2. be integrated to the platform. SetInstance is never useful since cdi is here for all impl. Only exception could be config which doesnt need it thanks a clean design "à la old EE". So plezse just stick to the platform and use cdi. |
That's like saying we should only use CDI managed instances of |
@jroper No, you got it wrong, this is like saying you can always have the provider available through an injection and compose it with java.util instances. Current reactive spec is not integrated/integrable to a CDI based app (so a MP app), it is just a plain standalone spec which is not consistent with the platform and doesn't bring anything to the platform itself (this work must be done IMHO but is more important than the spec taken alone, it must replace the fault-tolerance spec and integrate smoothly with CompletionStage already built-in in JAX-RS). So at the end the API should be platform driven and not isoalted and then we check how we can integrate. |
Which is from config, the only exception of all spec. |
No @rmannibucau the link @starksm64 posted is the setInstance method. Afaiu this merhod is not needed anymore, nowhere. Not even in mp-config. The point you likely mean is that mp-config is probably a rare example which cannot be CDI-first as it is needed during container startup already. This is actually a separated topic and we should split those 2 up in 2 tickets. |
Yep, misunderstood the link. Old osgi containers dont need it too (geronimo has 20 years and never need it) so i guess it is an ibm open liberty leak? |
@rmannibucau You're actually spot on when you say:
This is 100% true, MicroProfile Reactive Streams Operators doesn't bring anything to the platform itself. In isolation, it's not useful. However, it's necessary for other specs, for example MicroProfile Reactive Messaging, which entirely dependent on CDI. It's MicroProfile Reactive Messaging that provides the engine to the messaging builders that users return, and this design is necessary because different streams need engines configured in different ways, for example, providing access to different CDI scopes, depending on where they are used. If MicroProfile Reactive Streams Operators required an engine to be provided by the CDI container, then you would be stuck with using one engine, and all your streams would only be bound to one particular CDI context, and it wouldn't allow, for example, the messaging spec to ensure that streams ran in a messaging CDI context while a WebSocket spec ensured streams ran in the request context. There's been a lot of discussion about exactly where MicroProfile Reactive Streams Operators fits in the context of MicroProfile, including the very things you've been mentioning. It integrates well into CDI, but not by itself, it integrates well as used by the other specs. |
@jroper hmm, lead me to 2 comments:
|
CompletionStage is for single values. For streaming, CompletionStage is not adequate. Please read https://github.com/eclipse/microprofile-reactive/blob/master/approach.asciidoc to get the full picture. |
I read it and understood it as an already outdated spec. Basically completionstage is for one time emittion (of one or multiple result values through composition), subscriber and friend of current java version solve the multi values point and you can evzn, as done in big data world, just do a stream mixed with completionstages in java 8 to not wait for it. So at the end the spec adds a lot of complexity and layer which will prevent other qpecs to not use this one for pretty much no gain.....and it still doesnt require this old spi since it is this topic |
It doesn't address all the concerns of this issue, but during the Architecture call we agreed that SPI packages should be present in a separate SPI artifact and not present within an API artifact. |
In today's architecture meeting there was agreement that this issue can now be closed |
Many MP specs provide an SPI for implementations to provide implementations of some API interfaces. Usually, the following pattern is used:
setInstance
method (to simplify providing an implementation in an OSGi environment)Several issues have been raised to remove the
setInstance
method from the API artifact, claiming that separating it into an SPI package within the same artifact isn't enough to prevent API consumers using it: eclipse/microprofile-rest-client#103, eclipse/microprofile-open-api#224, eclipse/microprofile-config#364The text was updated successfully, but these errors were encountered: