Pavel,
thanks for picking up my issues.
About rx() and async(): I didn't liked the introduction async() methods from day one, so similarity to it is a negative argument from my view. ;-)
About type safety: The one and only _good_ type safe way is to ask RX framework vendors to agree upon a common API. As they did not, ANY solution will be bad.
About Future<T>: Compare the current code with my proposal and count the number of characters JAX-RS users have to write, and compute the difference in percentage. You will notice that the boilerplate for many applications will get reduced by about 20%. Also you will notice, that code is much cleaner. And, BTW, I will _not_ file a separate issue for that. The reason is that for me, RX is not just Client-API-RX, so I do _not_ want to separate these discussions.
-Markus
From: Pavel Bucek [mailto:pavel.bucek_at_oracle.com]
Sent: Sonntag, 15. Januar 2017 19:05
To: jsr370-experts_at_jax-rs-spec.java.net
Subject: Re: JAX-RS Client Reactive API review
Hi Markus,
thanks for your response. Please see inline.
On 13/01/2017 23:56, Markus KARG wrote:
Pavel,
thank you for sharing the code examples.
In fact I don't know if you actually want us to repeat our arguments, but here is my personal judgement of the proposed RX API:
I noticed the discussion already started in the other thread :-) Thanks for summing that up here!
* The decision to make CompletableStage the default and most simple way to use RX in JAX-RS is appreciated, as it will never go away, due to de-facto being the sole RX interface in the Java world which is part of the JRE and / or any JCP standard currently and in near future.
there is really no other option right now.
(we are thinking about including "Flow-like" classes into some parts of the spec, for better future adoption - since we know, it WILL be there .. but that will be discussed in separate thread).
* I don't like the rx() method which changes from "synchronous" to "reactive" invocation. I doubt that anybody would love to repeat rx() again and again, and I doubt that people want to write programs that shares sync and rx style invocations within the same code block. My counterproposal is to provide an additional RxClient class which always produces reactive invocations, and keep the normal Client untouched.
I see what you mean, but what about async(), which is already on the client? It is almost similar in terms of the way how the API was extended.
Also, there was similar enhancement related to nio, with the same approach - adding NioInovker, returned with method nio(). I know that this could be handled differently (and we will discuss that in future review), but it seems to me as "already established" way how to enhance JAX-RS client.
User can always create factory method and don't need to write the same code over a and over again. I believe this is already happening, at least in the code I saw.
Doing separate client for reactive programming style is interesting idea, but I believe it wouldn't fit into JAX-RS - when I thought about this, it was too similar to existing client, but would most likely require another copy of WebTarget, Invocation, etc., which doesn't seem to be worth it.
* I don't like the need to write down the type of custom RX invoker type again and again. I doubt that people really will write software which shares CompletableStage, RxJava, Guava etc. within the same code block. I assume that people instead will write full applications in *either* CompletableStage *or* RxJava *or* Guava etc (so they decide per app, not per invocation). The resulting application source code will be easier to read and potentially less error-prone if there instead would be an SPI to replace JAX-RS's reactive provider at deploy time, e. g. from the default CompletableStage to RxJava Observable.
I completely agree that having to add rx(..) method parameter over and over again is not elegant solution, but I don't know how else that can be solved. If we want to continue to have client statically typed (and I assume we do), there must be a way how to say "give me Observable" and the implementation must know which type users wants (to use correct provider - I already replied on this on the other thread). Then it seems that it is not really different, when we are providing class/instance of RxInvokerFactoty or a type, which should be returned, since both do define the same.
(the provider approach is more complicated, since it would require the provider to be registered, so another "state" which needs to be set and potentially lots of hours spent on "why is rx(Observable.class) not working?").
* The proposal only contains reactive support for the client API. I want to rise the question whether it might be a good idea to also extend server-sided async support at the same time. My proposal is to allow Future<T> as a return type of resource methods, so it is pretty easy to do concise async code like the example below (if we enforce CompletableFuture instead of CompletableStage). The idea is to get rid of the async boilerplate and the need to explicitly wait for the result of the RX invocation, but just to tell JAX-RS to set up the precessing chain and let it execute in the background by Java EE's default ManagedExecutorService (hence outside of the scop of the code block, hence outside the HTTP thread pool):
@GET public Future<List<String>> collectResultsFromOtherServers() {
return client.target("remote/forecast/{destination}")
.resolveTemplate("destination", "mars")
.request()
.header("Rx-User", "Java8")
.rx()
.get(new GenericType<List<String>>() {
});
/*
* Certainly the actual idea is to combine many parallel invocations instead of simple returning one!
*/
}
Even if there is some combined result, how would that be different compared calling Future.get() on the combined one and returning the result directly? If we are returning Future, there is always a "Future.get()" invocation, which is blocking by definition.
I can see the enhancement if we would restrict this to CompletionStage (not CompletableFuture), since there is no get() method, so the impl. could register a callback and be notified after the event is done - in reactive, non-blocking fashion). Or we could return something like Flow.Publisher.. we are already thinking about something similar for Client SSE API, but that is different discussion I guess.
Since it seems like current proposal brings enough discussion topics on its own, and this seems to be separate issue, can you please file this as a issue against JAX-RS on java.net? We can get back to this later.
Thanks!
Pavel
-Markus
From: Pavel Bucek [mailto:pavel.bucek_at_oracle.com]
Sent: Freitag, 13. Januar 2017 21:36
To: jsr370-experts_at_jax-rs-spec.java.net
Subject: JAX-RS Client Reactive API review
Dear experts,
please review following wiki and APIs:
https://java.net/projects/jax-rs-spec/pages/RxClient
All added classes related to Reactive Client APIs are linked from that page, but let me allow a short recap.
JAX-RS Client is being extended by the ability to provide a way how to process responses in reactive fashion. The change consists of:
- adding rx(...) methods to Invocation.Builder
- defining RxInvoker
- allowing users to extend this API by providing RxInvokerProvider
Specification will mandate implementation for CompletionStage from Java SE 8.
Client code examples:
- basic use
CompletionStage<List<String>> cs =
client.target("remote/forecast/{destination}")
.resolveTemplate("destination", "mars")
.request()
.header("Rx-User", "Java8")
.rx() // gets CompletionStageRxInvoker
.get(new GenericType<List<String>>() {
});
cs.thenAccept(System.out::println);
- using custom RxInvokerFactory (this is little artificial, since the factory just returns CompletionStageRxInvoker, but support for Observable from RxJava or ListenableFuture from Guava can be done in the exact same manner)
CompletionStage<List<String>> cs =
client.target("remote/forecast/{destination}")
.resolveTemplate("destination", "mars")
.request()
.header("Rx-User", "Java8")
.rx(CompletionStageRxInvokerProvider.class)
.get(new GenericType<List<String>>() {
});
cs.thenAccept(System.out::println);
Source links:
-
https://github.com/jax-rs/api/blob/2.1-m02/jaxrs-api/src/main/java/javax/ws/rs/client/Invocation.java#L298
-
https://github.com/jax-rs/api/blob/2.1-m02/jaxrs-api/src/main/java/javax/ws/rs/client/RxInvoker.java
-
https://github.com/jax-rs/api/blob/2.1-m02/jaxrs-api/src/main/java/javax/ws/rs/client/RxInvokerProvider.java
Examples & tests:
-
https://github.com/jax-rs/api/blob/2.1-m02/jaxrs-api/src/test/java/javax/ws/rs/core/RxClientTest.java
-
https://github.com/jersey/jersey/blob/2.x/core-client/src/test/java/org/glassfish/jersey/client/ClientRxTest.java#L86
The last link is to the Jersey repository. Jersey version 2.26 will be JAX-RS 2.1 RI and branch 2.x is where the development will happen. Jersey 2.26-b01 (which is being released right now) implements all rx(...) methods; feel free to test/evaluate it there.
Looking forward to your feedback!
Thanks and regards,
Pavel