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
>