-
Notifications
You must be signed in to change notification settings - Fork 114
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
Java SE Bootstrapping API #619
Conversation
* | ||
* This method will not return until the JAX-RS application is terminated. | ||
* | ||
* @param rootURI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually common to provide an URI
to configure the endpoint? I'm not against it, I just didn't see this before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more general than a URL. Do you think URL would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would definitely prefer URI
over URL
. It's just that most API's I know of don't use URL
/URI
but some other way to specify all the required parameters for bootstrapping.
The most interesting question is: Which information does an implementation need for bootstrapping?
- A TCP port number
- Which interface to bind to
- Which protocol to use (HTTP/HTTPS/AJP)
- Socket Timeouts?
- TLS certificate to use for HTTPS? (just an example. Most likely not relevant for most apps)
Most if this can be represented with an URI
. But maybe implementations would like to support advanced options. In this case the API should provide a way to pass these options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the minimum is the root URI and the Application, obvisouly. Certainly we could add more parameters if needed. Which one are essential and supported by all vendors?
BTW, Jersey is using an URI:
public static HttpServer createHttpServer(final URI uri, final ResourceConfig configuration) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the root URI would provide us with the protocol, port and host address. And I agree that this is the minimum. Perhaps we just need some kind of map or properties parameter so that implementations can support advanced options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chkal I simply do not see any need to synthetically restrict the key to String
as it will work pretty well with any kind of Object. Unless the key must be human-readable (i. e. it solely serves the purpose of technically identifying the value in the map), Object
is pretty sufficient. So every vendor (and the API itself) can define static Object
s for the keys. This is, BTW, the best performance you can gain with comparisons, as it will never use character comparions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spericas I agree that binding to 0.0.0.0 would be weird when using an URI
. However, apart from that it feels elegant to use an URI
, because it naturally includes the protocol, the port and the base path. But as mentioned above I'm not 100% sold on this idea. I never saw an URI being used in this context, so I'm still in the process to make up my mind if I like it or not. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkarg Understood. I see advantages of using Object
as you can for example use enumerations as keys. But one disadvantage of it is that you are forced to use some kind of static Object
as the key if you configure the runtime. So you cannot read values from config files, environment variables, etc anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spericas I had chosen URI
because Jersey uses that, see https://jersey.github.io/apidocs/latest/jersey/index.html. But I am also happy with Map
.
@chkal The question is whether persisting config is our problem? If someone wants that, he can simply write some load
method which replaces any text by predefined singletons. This is very beneficial as it would throw an "unknown configuration option" right at the time of parsing the text file, way before the app even tries to bootstrap the runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quipsy-karg Sure, persisting isn't our problem. But we we should keep the API simple for real world use cases. And loading config property from external sources is a real world use case.
* @param application | ||
* The application to start up. | ||
*/ | ||
default void bootstrap(final URI rootURI, final Application application) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I like having this on the JAXRS
interface itself. Maybe we should add some other interface for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same thought. But is a new interface the right choice? Or de we want to use RuntimeDelegate?
* | ||
* @since 2.2 | ||
*/ | ||
interface JAXRS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a better name than JAXRS? A Bootstrapper
, or an ApplicationFactory
is more descriptive perhaps? Should this be an Interface? Abstract class could have #bootstrap
method protected, if do not want to call it outside from #start
.
I'd slightly prefer a RuntimeDelegate
method such as #createApplicationFactory()
, to avoid another static method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was chosen in analogy to the existing classes JAXB
and Persistence
which serve the same purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think #createApplicationFactory() would be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like JAXRS
. And I agree with @mkarg that it is consistent with JAXB
and Persistence
,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think current Java API trend is to only uppercase the first letter (see Jsonb). That would leave as with Jaxrs.
Don't have a preference. Just a thought to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggam Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot see a trend. This sounds more like personal taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the use of the JAXRS name, but I understand the rationale for it. However, why isn't the implementation of the methods in this class not using RuntimeDelegate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spericas Good question. Let me change that.
I can see the rationale for this, but at the same time I think this work should probably have a larger scope. JAX-RS is just one part of a microservice, there are components like configuration, health checks, metrics, etc. that are also important. IMO, we need to have a discussion about the larger picture and potentially new dependencies before exploring this bootstrapping API. |
I do not think that this particular PR has to be withhold until more Microservice APIs are discussed. See, the aim is to have a vendor-neutral API for bootstrapping JAX-RS, not more, not less. Yes, it is useful for Microservices. But it also is useful without any that. In fact, we are using Jersey-onley Microservices currently, and this is the single line of code that makes our code Jersey-dependend. I really want to get rid of it, and I do not see why I have to wait just because other useful things are in the queue. |
@mkarg I disagree, we absolutely need to have those discussions before designing a likely incomplete/flawed API. There was a similar comment to mine (maybe @andymc12?) in relation to MP Config and the declarative client API. Bootstrapping JAX-RS on SE requires support good ways of specifying server configuration, which I don't see here. |
@spericas Can you elaborate a bit what actual risk do you see by standardizing the already existing Java SE bootstrapping APIs without further discussing Microservice APIs? I just cannot see what you mean with "flawed" here, as I did not propose additional features, but just wrapping existing features which only deal with the very initial bootstrapping. What risk do you see? What is flawed here? |
Couple of thoughts:
|
@mkarg A lot of my points are already in @jansupol last comment. Elaborating on one of them, bootstrapping a JAXRS app requires a significant amount of configuration (SSL, authn/authz, ports, network interfaces, CDI support, health checks, etc.). This is typically larger that what people want to specify/hardwire in code. That is the reason for specs like MP Config. Should JAX-RS define an external source for this config? Not sure we should be in that business, we should instead leverage other specs. Also, back to the microservices use case which is a motivation for this work if I understand this correctly, config can be provided externally: e.g., via config maps in kubernetes. In summary, sure we can define an API with a simple URI, but I don't see it being useful beyond hello-world use cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to have this API, however I tend to agree with @spericas that we need to properly think about all the implications. We're going to have the new API basically forever, so while we can certainly expand it later, doing it right since the beginning would be great.
Btw, this reminds me of JAX-WS 2.2 and its HTTP SPI http://download.oracle.com/otn-pub/jcp/jaxws-2.2-mrel3-evalu-oth-JSpec/jaxws-2_2-mrel3-spec.pdf (section 6.6)
I agree that how to configure the runtime is one of the key aspects of such an API. There are many possible sources for configuration values we could support (like MP Config). However, I'm not sure if we really should add a dependency on MP config. I just had a crazy idea I would like to bring up. Imagine a bootstrap API like this: public interface JAXRS {
static void start( Application app, Map<String,Object> config ) { ... }
} This would allow the developer to provide a configuration map just like when configuring JPA or other frameworks. I think this is really common. But what about if the developer could also provide a function which resolves configuration values: public interface JAXRS {
// [...]
static void start( Application app, Function<String,Object> provider ) { ... }
} This could provide a nice and clean way to integrate with all possible configuration APIs. Just provide a function to delegate to your config source of choice and you are done. JAXRS.start( myApp, key -> ConfigProvider.getConfig().getValue(key, Object.class) ); Just an idea. ;-) |
+1 for providing a map; 0 for provider functions: the bootstrap is called exactly once per application, so there is no real benefit of the additional complexity, and the application programmer MUST write a rather complex intermediate lambda anyways to filter which properties he wants to set or not. |
Sure, but having to read each configuration property from an external source just to populate a map which you then pass to the bootstrap method is ugly boilerplate you have to repeat for each app. And also, which complexity? The complexity of having an additional bootstrap method with anther signature?
No, you won't need that. The function is called by the JAX-RS implementation only for support configuration keys. So there is no need to do any filtering. |
@chkal I am not against what you propose, I just am not finally convinced that it is really better. |
0290cba
to
6e7a836
Compare
I tried to adopt as many as request changes as I could. Please feel free to comment. I hope you like it better now. :-) Regarding compatibility with MP Config I'd rather go with @chkal's idea of callbacks to abstain from any direct reference to any particular technology. Regarding CDI and 3.0 I'd like to have Java SE bootstrapping ASAP, i. e. in 2.2. |
The use of a config provider in the form of a lambda is definitely a step forward, but the API still feels a bit clanky to me (think about actual implementations of those config providers). If we are going to define properties for compatibility, which we have to, why not use the builder pattern like in the Client API for more natural COC and ease of use? Here is how I envisioned this bootstrap API:
Also, the use of a config builder will allow integration with "other config sources". This feels more modern and also better aligned with recent APIs like those in the MP. |
When using the builder pattern, an implementation of JAX-RS runtime cannot add custom properties (like Jersey: the type of HTTP engine to use either JDKHTTP or Grizzly for example) -- he will be limited to the properties we define upfront in the API. Also when someone wants to simply load the config from a text file or from the MP config API he has to write rather complex glue code instead of simply passing key-value pairs. |
That's not true, you can always have a generic property(key, value) in your builder if necessary (but of course, if you use that you cannot guarantee compatibility which is the original motivation for the API). As for the implementation for loading a config, you're just pushing that to the application by providing a lambda (config provider). |
* Invoked in Java SE environments to start the provided application at the | ||
* specified root URL. | ||
* | ||
* This method will not return until the JAX-RS application is terminated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is a JAX-RS application terminated? Would you want to add a new stop()
or close()
method to the Application
class? Alternatively, maybe the JAXRS.start
method could return immediately after starting the application asynchronously, and then you could add a JAXRS.stop
method to terminate the application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PR only proposed to wrap existing vendor-specific technology under a common API hood. AFAIK not all JAX-RS runtimes do support explicitly stopping an application. So your proposal would be to add a completely new feature. If all vendors support that, I am fine with it. But until then, I cannot unilaterally set up a PR for that.
So question to @jansupol, @asoldano, @deki: Do you like to add support for explicitly stopping a JAX-RS application as proposed by @andymc12? I would be more than happy to add this to my PR in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mkarg - I'm mainly having trouble understanding how the start()
method works - or more specifically how the method ever returns. And possibly my brain is malfunctioning because I've been so deep in the Java EE (server-side) mode for too long that I can't process Java SE patterns. ;-)
Here are a couple scenarios where I think a blocking start
method with no spec-defined way to stop the app could be troublesome:
-
User wants an app to read from a database. In their
main
method, they set up some database connections then callstart
to load up the JAX-RS front-end. At some point, they will want to stop the process (perhaps to deploy a new version of the app, or do DB maintenance, etc.). They could just kill the JVM process, but that won't gracefully disconnect from the DB - not a huge problem, but still messy. -
User wants to test their app before deploying it to a Java EE server. They set up mock resources (like WireMock or MockServer, etc.) that run in separate processes. Once their testing is complete, the user would need to kill not only their Java SE JAX-RS process, but any other processes that it may have started - instead of leveraging the convenient APIs for stopping those processes.
I'm not sure if CXF has a way to stop an application (outside of the runtime it is running in - Tomcat, OpenLiberty, etc.) so it might not have a mechanism to return from this method as it is today. That's not necessarily a problem - but then we should probably update the spec to indicate that implementations should provide a mechanism for stopping apps.
Hope this helps...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andymc12 I share your thoughts, but again, I am only trying to wrap the current behaviour of the existing products as-is. The start method will only be implemented IF the existing products CAN be started on Java SE (this is AFAIK not mandatory as of JAX-RS 2.1+). IF an existing product hasn't a way to be stopped BUT supports Java SE, then effectively effects 1 and 2 will happen actually, independent of my proposed API or not -- again, I am not specifying a new feature or behaviour, I only wrap EXISTING products. So again my question to all vendors: Can your existing products be stopped explicitly, yes or no? If yes, I can add a stop mechanism in this API. If no, are you willing to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In think in most cases ShutdownHook is used in this case and CXF is shutdown by closing the Spring context. Should be also possible to implement it for non Spring scenarios, org.apache.cxf.endpoint.Server has stop and destroy methods. Comparing to JAX-WS javax.xml.ws.Endpoint has also a stop method, so I'm open to have something similar in JAX-RS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if we add |
+1 for using a builder with a generic |
@omnijava So you like to talk about this new JAX-RS feature at Oracle Code One? Otherwise I wouldn't see any relation to this thread. |
9eccfc2
to
5abaf7a
Compare
Seventh DraftI have pushed the seventh draft of this API to my repository. Also the PoC (Jersey 2.28-SNAPSHOT) is updated, and I hacked a very eary PoC using RESTeasy. Please post your feedback as usual. There had not been much feedback on the last draft so far, so I concentrated on authoring examples and JavaDocs mostly. My focus will be on tests for the next draft, unless you figure out commonly supported features of actual JAX-RS implementations that you want to cover here (which I do not assume looking at the existing Jersey and RESTeasy code). In fact, I assume the upcoming draft eight might be the final one before I open this PR for voting. Any comments welcome. :-) |
Eighth DraftI have pushed the eighth draft of this API to my repository. Also the PoC (Jersey 2.28-SNAPSHOT) is updated. Please post your feedback as usual. There had not been much feedback on the last draft so far, so I first planned to concentrat on tests and javadocs. But as a reaction to the feedback of my presentation at Java Forum Stuttgart, and in reaction to Heiko Rupp's presentation on Microprofile APIs, I decided to add another feature first: Bulk-Loading of properties, particularly from external configuration mechanics like Microprofile Config API. So this draft actually adds the following two features, but still no tests nor improve Javadocs.
So I think tests will be there in the next draft, and possibly the ninth will be the final one before I open this PR for voting. Having said that, I particularly want to ask vendors for feedback at this point, because the time comes near when we (hopfully) adopt this feature proposal, hence you will try to implement it. As there might pop up issues then, it is wise to start early, possibly using a night build (a self-made one, as the EF must not publish theirs as we learned). Any comments welcome. :-) |
db788d2
to
b93cc3a
Compare
9a03fb6
to
4ee7a32
Compare
4ee7a32
to
6314912
Compare
Signed-off-by: Markus KARG <markus@headcrashing.eu>
6314912
to
3007903
Compare
Thanks to everybody for the inspiring discussions and great improvements of the original proposal! The proposal reached a state where no substantial objections existed, the WIP covered the intended target, and the PoC worked fine. There had been no comments on seventh and eighth draft, and no issues had been left unaddressed. Hence, it is ripe for voting. All committers please use the Github review tool to approve this PR within the next two weeks. Note that accepting this PR does not mean that it is carved in stone. There is much room to add PRs ontop later if we identify a need to extend the API further, as 2.2 is months away still. The PoC (Jersey) reached a state where it is near production quality. As soon as the API is accepted, I will propose it for inclusion in Jersey 2.28. Until then, feel free to use the published branch if you want to see the API in action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mkarg. As already mentioned in my previous review, I'm very happy with this version. And I agree that we can do more fine tuning (if required) even after this one is merged!
Hm... is everybody on vacation, or why is there only a single approval within one week...? ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder - my mind has been on vacation lately! ;-)
Having two +1s, and as this PR is under active discussion since months, I decided to merge it today, even before the end of the announced review period, so I can go on with my work in the Jersey implementation. If anybody really wanted to -1 (he must have a good reason after all these months), please do so, and I will revert the merge then. |
This PR implements a vendor-neutral way to bootstrap JAX-RS applications in pure Java SE environments (i. e. making the application self-contained, without being powered by an application server product). It just provides a common API for starting up the application and runtime, but does not make any contstraints about supported features or technology.
Signed-off-by: Markus KARG markus@headcrashing.eu