Non-Mono overload for a non-streamable argument

[salesforce]

Non-Mono overload for a non-streamable argument

[rmichela]

I've taken a first stab at implementing simple unary request parameters. Please take a look @ktoso @bsideup .

[cbornet]

I had a look at other rx-streams libs (eg. spring-data) and they indeed expose both a reactive and non-reactive argument API. Moreover, the reactive argument method takes a Publisher which I believe is better as it’s more generic. So we should do the same. WDYT @rmichela ?

[rmichela]

I'm sort of on the fence with this one.

I can appreciate the elegance of method signatures that align with the types you have, but I'm not convinced that generating (and maintaining) a second Mono/Single client stub method to save from typing Mono.just( is justified.

Perhaps I'm being overly conservative. Please feel free to challenge me. 😉

[ktoso]

I believe I agree with @rmichela.
In fact, in akka/akka-grpc this is what we choose to do as well;
Unary calls are represented by Futures or CompletionStage (well, or just parameter to the to-be-implemented-function); and streaming calls are represented by our Reactive Streams counter-parts which are Source in this specific example.

https://github.com/akka/akka-grpc/blob/master/plugin-tester-scala/src/main/scala/example/myapp/helloworld/GreeterServiceImpl.scala#L17

Keep it simple :-)

[ktoso]

I see what you mean now; correct, incoming parameter is just a T, not a Future[T]. We saw no need to keep the potentially delayed thing there -- once we gather the element, we invoke user code, no reason to keep it wrapped there indeed.

[rmichela]

Ok. If this change aligns with other reactive libraries, it makes sense.

[rmichela]

Interesting idea...care to submit a PR? 😁

[bsideup]

@rmichela maintainance is of course a good argument, but for this one IMO it's rather minor :)

reactive-grpc is a great library to generate reactive-ready clients from protobuf definitions.
In fact, in one of our projects we suggest to use it "as is" without providing any additional client libraries around it.

But when you look at it, it looks strange that the user has to wrap arguments with Mono:
https://github.com/bsideup/liiklus/blob/dee36132855870a37f87b8c8dd42cc457d4d2118/examples/java/src/main/java/com/example/Consumer.java#L59

I saw your compose-style usages, but it confuses the end users.

Other reactive libraries (like reactor-netty) offer non-reactive arguments when it's not needed:
reactor-netty example:
https://github.com/reactor/reactor-netty/blob/930d748d51fa3b6129d4346fe3b062b654fd3818/src/test/java/reactor/ipc/netty/http/ClientServerHttpTests.java#L291
akka streams http example:
https://github.com/akka/akka-http/blob/ca28e2db878487c2a4bd7d34cb78139f64a4d70d/docs/src/test/java/docs/http/javadsl/HttpClientExampleDocTest.java#L182

it's also possible (in future) to optimize such calls and remove Mono -> un-Mono chain when the source of argument is not reactive.

Forcing it to be Mono creates unnecessary feeling that the argument has to be cold, while most of gRPC arguments are hot inline builders

TL;DR:
I'm a huge fan of reactive APIs but IMO they should be reactive only when necessary.

maybe @smaldini @simonbasle @akarnokd @ktoso have something to add.

[bsideup]

After some thoughts I even think that the generated implementation should look like that:

public Flux<...> doSomething(Mono<SomeType> argument) {
    return argument.flatMapMany(this::doCall);
}

public Flux<...> doSomething(SomeType argument) {
    return doCall(argument);
}

This way there is no Mono -> un-Mono chain and it supports both scenarios :)

[bsideup]

@ktoso but the argument is not Future, right? While reactive-grpc always demands the arguments to be Mono (Future-ish)

[bsideup]

@rmichela looks good as a first step, removes a bunch of pain already 👍

[rmichela]

looks good as a first step

Is there a second step? 😉

[bsideup]

@rmichela
bsideup/liiklus@20fd7ef

So. Much. Better. 😍 Thanks!

A second step, as I see it, is to do it like this:

public Flux<...> doSomething(Mono<SomeType> argument) {
    return argument.flatMapMany(this::doCall);
}

public Flux<...> doSomething(SomeType argument) {
    return doCall(argument);
}

Good for performance critical apps