Client power user API #201

johanandren · 2018-05-07T14:08:05Z

Fixes #191

Went for

client.method()
  .bladi()
  .dadi()
  .execute(param)

instead of

client.advanced.method(param)
  .bladi()
  .dadi()
  .execute()

johanandren · 2018-05-07T14:09:16Z

No usage samples yet, but the "normal" calls are now expressed through the new RequestBuilder version of the method, and interop-tests passing.

octonato · 2018-05-07T14:14:28Z

runtime/src/main/scala/akka/grpc/scaladsl/RequestBuilder.scala

+
+  def addMetadata(key: String, value: String): RequestBuilder[Req, Res]
+  def withDeadline(deadline: FiniteDuration): RequestBuilder[Req, Res]
+  def execute(req: Req): Res


What about invoke instead of execute?

No strong opinion either way

Me neither. ;-) Lagom uses invoke hence my slight preference.

Btw, that API style means that we always have a lifted API representation instead of a switch giving access to the lifted/advanced one.
I thought we wanted to provide a non-lift, straight, variation for the most common cases.

Yeah. Not sure that it is always available rather than hidden behind a switch is a problem though. The old non-lifted API is still there (and should be the default)

Aligning with Lagom seems like sensible motivation for invoke :)

Ah ok, now I see it. Because in gRPC there is always one param, we can overload the method with a paramless variation that gives access to the lifted version. Neat! Much better than a switch.

raboof · 2018-05-07T14:55:06Z

Right! Moving the parameter to the end of the chain makes a lot of sense: this way a library user can even reuse the RequestBuilder for multiple requests that have the same characteristics. That seems like a cool advantage and I don't see any disadvantage.

raboof

Very nice so far!!

raboof · 2018-05-07T16:20:17Z

codegen/src/main/twirl/templates/ScalaClient/Client.scala.txt

  }
+
+  /**
+   * For access to method metadata use the parameter less version of ${method.name}


('parameterless')

raboof · 2018-05-07T16:27:21Z

runtime/src/main/scala/akka/grpc/scaladsl/RequestBuilder.scala

+  }
+
+  def withDeadline(deadline: FiniteDuration): RequestBuilderImpl[Req, Res, Ret] = {
+    copy(options = options.withDeadline(Deadline.after(deadline.length, deadline.unit)))


Nice, we don't support this at the server side yet (#188) but since this is 👍

Yeah, just picked this randomly to have some functionality to provide through the request builder.

raboof · 2018-05-07T16:39:11Z

runtime/src/main/scala/akka/grpc/scaladsl/RequestBuilder.scala

+  delegatedExecute: (Req, CallOptions) => Ret) extends RequestBuilder[Req, Ret]() {
+
+  def addMetadata(key: String, value: String): RequestBuilderImpl[Req, Res, Ret] = {
+    // FIXME support values other than string values?


do 'options' correspond to 'headers' or is there more indirection there?

I think there is a bit more indirection, there is also some marshalling infra in the grpc library around this. Not sure if we care about that though, maybe strings are good enough for now, and also avoids tying us down to something client specific.

It's annotated as an experimental API, but so is most of these CallOptions.

raboof · 2018-05-07T16:43:08Z

codegen/src/main/twirl/templates/ScalaClient/Client.scala.txt

+  def @{method.name}(): RequestBuilder[@method.parameterType, @method.returnType] = {
+    val descriptor = @{method.name}Descriptor
+    @if(method.methodType == akka.grpc.gen.Unary) {
+      RequestBuilderImpl.unary(descriptor, channel, options)


Do we want to organize the template so that the indentation of the generated code makes sense? No real preference here.

Ah, I thought I did follow that, since it is how it was, but maybe having the templates readable is more important for maintainability than not having too many indentation levels in the generated sources?

johanandren · 2018-05-08T07:38:08Z

One thing I couldn't figure out was a reasonable way to deal with response metadata, for single response calls it may be relatively straight forward API wise (Future[(Resp, Metadata)]) but for streaming data it just doesn't make sense to put it inside of the stream, as you'd potentially want to act on it before consuming the response stream.

Any thoughts around that?

ktoso · 2018-05-08T07:39:58Z

I had proposed a wrapper type for that I believe; GrpcResponse(stream or future).with(all the stuff), WDYT?

I would avoid forcing people to return Future[(A, B)] since not so nice for users to create such one (they have to do the "ziping", rather than just "return the meta stuff and the future/stream"). A custom type like this would also allow us to handle "but the medatada is a Future as well", if we ever had to do this, without changing signatures.

johanandren · 2018-05-08T09:41:04Z

Thinking about this some more, maybe the primary use case will be to set metadata on the request, not deal with response metadata, so perhaps there should be the current "simple" invoke(req) and then a more complete but also more messy invokeWithAnnoyingReturnType(req)?

octonato · 2018-05-08T09:51:32Z

I think a better way of doing that is to have a method that changes the return type. That's also how it was done in Lagom.

For instance:

val res: Future[Res] =
  client.method()
    .addMetadata(key, value)
    .invoke(param)

val res: Future[GrpcResponse[Res]] =
  client.method()
    .addMetadata(key, value)
    .withResponseHeaders
    .invoke(param)

This requires two impl of RequestBuilder and the method withResponseHeaders works as a one direction switch.

johanandren · 2018-05-08T11:38:32Z

I don't see why a stateful/extra type is better than an additional invoke method with different return type. Am I missing something? Just to align with Lagom?

octonato · 2018-05-08T13:24:33Z

Actually, the Lagom implementation has two methods:

def handleResponseHeader[T](handler: (ResponseHeader, Response) => T): ServiceCall[Request, T]

def withResponseHeader: ServiceCall[Request, (ResponseHeader, Response)] =
    handleResponseHeader((_, _))

The with variation is just a convenience to expose the headers.

The handle one is very flexible. You can use it to transform the Response using some header information. So, basically, you define the transformation before calling invoke. A method like invokeWithHeaders won't allow it.
(https://github.com/lagom/lagom/blob/master/service/scaladsl/api/src/main/scala/com/lightbend/lagom/scaladsl/api/ServiceCall.scala#L55)

Moreover, Lagom's ServiceCall is also used on the server side. Same API everywhere. And response header manipulation is certainly useful on the server side.

Correction: I previously said we needed two RequestBuilder impl. That's not true. It's just about applying transformations, ie: T => GrpcResponse[T].

TimMoore · 2018-05-18T01:50:24Z

Yeah, I think it is pretty useful to be able to write decorators that can manipulate either/both the request and response metadata and payloads. I don't see a way with this API to be able to write a decorating function that can intercept the response without using internal APIs.

johanandren · 2018-05-21T13:35:14Z

Hmm, I still don't get it, why would a special type with transformations be better than just having a Future[ResponseAndHeadersType]/CompletionStage<ResponseAndHeadersType> which you can transform in exactly the same way as all your other async thingies and that you already know the API of?

octonato · 2018-05-23T15:19:08Z

@johanandren, I’m not sure of all the motivations of it in Lagom, but from what I can infer from the API, it allows you to change the return type using some information available on the metadata.

What I think is most important is that the client should only receive the headers if required. The Lagom API allows you to send headers, but to not necessary receive the response headers. This kind of flexibility is important because we let the user augment the API surface when needed.

The opposite is also true. In Lagom is possible to make a call and expose the response headers without having to send request headers.

I think that the case in Lagom is simply because one is built in terms of the other. First you provide a method to expose the response headers and modify the return type.

def handleResponseHeader[T](handler: (ResponseHeader, Response) => T): ServiceCall[Request, T]

then you build another one using identity function.

def withResponseHeader: ServiceCall[Request, (ResponseHeader, Response)] =
    handleResponseHeader((_, _))

Now you have the choice to use one or another according to your needs / preferences. I think that's just that.

johanandren · 2018-05-24T07:32:00Z

I think all those cases you describe should be covered by having two methods on the builder: the one that currently exists and then one that returns a Future[ResponseWithHeaders]. Just calling map on that then allows you to transform the response to any type you want while also looking at the response headers.

octonato · 2018-05-24T08:16:40Z

Sure, but does that mean that in case of streaming we will get a Future[StreamResponseWithHeaders[T, R]] from each I could read the headers and the Source?

This sounds good and it's worth experiment. I has some implications though.

If the general API returns Source[T, R], the power user API will have to return Future[StreamResponseWithHeaders[T, R]].

It also means that there are effectively two calls, one to get the Future and one when we effectively materialize the Source, but I guess we can't avoid it. Unless we return Source[(Headers, T), R] which has an obvious overhead penalty.

And then we may have a another problem, for the moment mostly theoretical. This two calls may create a situation where the metadata differs between them.

octonato · 2018-05-24T09:35:17Z

For completeness and clarity.

The implications I mentioned above (two calls) also holds for the handleResponseHeader API. I didn't mean to say that using Future[ResponseWithHeaders] was forcing us into that situation.

octonato · 2018-05-24T09:43:28Z

Just a crazy thought.

The only way to associate headers to a stream and make sure that they relate (make part of the same call) is Source[(Headers, T), R]. We also want to keep the semantics of Source and be able to re-run it (make the call again) whenever we want.

So, maybe...

sealed trait Headers
case object EmptyHeaders extends Headers
case class NonEmptyHeaders extends Headers

Then is up to the implementers to decide when to send the headers. On the first element, on each element, etc.

Didn't thought thoroughly on the implications of this idea. Maybe stupid, maybe crazy, maybe good. I let this go through your brains first. :-)

raboof · 2018-05-24T09:53:05Z

I'm not entirely convinced we want to keep the semantics of Source and be able to re-run it (make the call again) whenever we want: I don't think we do that for Source-backed entities in HttpRequests either.

Source[(Headers, T), R] suggests each T may have headers, which is not the case (as it's the response that has (leading and trailing) headers, not each element in the response individually) - so from that perspective returning the future containing headers and source seems to make more sense.

octonato · 2018-05-24T09:55:37Z

suggests each T may have headers

You are totally right on that. Not a good idea.

johanandren · 2018-05-24T12:10:48Z

Internally the headers will arrive when the flow is materialized, and then there is a Future[Trailers] since those won't arrive until the stream completes. Haven't thought through how to provide this to the user yet, but the underlying stage I just wrote can at least be materialized multiple times and each materialization leads to one call - one set of metadata.

octonato · 2018-05-24T12:23:49Z

That sounds really good. Need to check later how this is being achieved.

ignasi35 · 2018-05-24T13:03:40Z

If a Source can be materialised several times causing a new call each time, then could headers and trailers be the materialised value? (I don't even know if that makes sense)

Source[T, (Future[Headers], Future[Trailers])]

johanandren · 2018-05-24T13:30:11Z

Yeah, likely. But I think a specific type capturing it would be better than a tuple. The alternative would be to side effect from mapMatValue to complete some promises/futures for then, that way it could be something like StreamingGrpcResponse(stream, futureHeaders, futureTrailers).

I think the matVal feels most natural

ignasi35 · 2018-05-25T11:54:17Z

I find counterintuitive that materializing the Source several times causes new calls without using client.method()...invoke(param).

IIRC Lagom's client produces a Source that's only meant to be materialized once and new calls only happen on new invoke. Also, in the 4 possible combinations (unary-basic, streamed-basic, unary-powerUser, stream-powerUser) I think this would be the only combination where materializing twice creates two calls, right? Or is a Source obtained using the basic (non-powerUSer) API also behaving like this?

Maybe renaming akka-grpc's invoke to callFactory or something that's not invoke could help make it more obvious what's actually happening.

johanandren · 2018-05-25T13:33:30Z

Both of the source variations could be materialized many times. If that is an issue we can protect against that and fail. I don't have a gut feeling about it being surprising or not yet, I'll see when the actual user API is in place (which is what I am currently working on).

ktoso

Looking very good :)
No strong opinion on the source running semantics yet... best to play around a bit and gather feedback on that one?

raboof

Looks great!

raboof · 2018-05-28T13:02:49Z

plugin-tester-scala/src/main/scala/example/myapp/helloworld/LiftedGreeterClient.scala

+      "127.0.0.1",
+      8080,
+      overrideAuthority = Some("foo.test.google.fr"),
+      None,


perhaps name this parameter as well?

ups, keep missing out on the plugin-testers as they are not part of the aggregated build

raboof · 2018-05-28T13:03:39Z

plugin-tester-scala/src/main/scala/example/myapp/helloworld/GreeterClient.scala

@@ -56,7 +66,7 @@ object GreeterClient {
      val responseStream = client.itKeepsReplying(HelloRequest("Alice"))
      val done: Future[Done] =
        responseStream.runForeach(reply => println(s"got streaming reply: ${reply.message}"))
-      Await.ready(done, 1.minute)
+      Await.ready(done, 1.minute) // just to keep sample simple - dont do Await.ready in actual code


Perhaps we should avoid it here as well then - but let's keep that for another PR

raboof · 2018-05-28T13:10:26Z

runtime/src/main/scala/akka/grpc/GrpcClientResponse.scala

+
+import scala.concurrent.Future
+
+// FIXME should we provide our own immutable/thread safe response metadata abstraction?


Indeed the upstream Metadata is not too nice, might be good to at least wrap it in some immutable interface?.

raboof · 2018-05-28T13:12:47Z

runtime/src/main/scala/akka/grpc/internal/RequestBuilderImpl.scala

+
+  def addMetadata(key: String, value: String): T = {
+    // FIXME Key is instance equal to allow for replacing of the same key but still also allowing multiple
+    // values with the same name-key, not sure if it is important we support that


Let's keep an eye out if we ever see that (multiple roles?).

If so another way to do it might be to have this method overwrite keys, and introduce a def addMetadata(key: String, value: Seq[String]) for multi-valued metadata

It turned out that what I thought was the same thing, the CallOptions.options and the metadata headers, was in fact not at all the same thing, so this is gone now.

raboof · 2018-05-28T13:13:09Z

runtime/src/main/scala/akka/grpc/internal/RequestBuilderImpl.scala

+  def options: CallOptions
+  def updated(options: CallOptions): T
+
+  def addMetadata(key: String, value: String): T = {


Since later the GrpcResponseMetadata contains headers, perhaps addHeader would make more sense here?

Yeap, I renamed when changing it into metadata. I realize I renamed it wrong though, changed it to withHeader as I thought it replaces, but I realize now that it should be additive and not replace, so add would be better.

raboof · 2018-05-28T13:14:01Z

runtime/src/main/scala/akka/grpc/internal/UnaryCallAdapter.scala

+ *
+ * INTERNAL API
+ */
+// needs to be a separate class because of CompletionStage error handling not bubling


bubbling ;)

Less doubling in my bubbling!

raboof · 2018-05-28T13:27:13Z

runtime/src/main/scala/akka/grpc/scaladsl/RequestBuilder.scala

+  /**
+   * Invoke the gRPC method with the additional metadata added and provide access to response metadata
+   */
+  def invokeWithMetadata(request: Req): Source[Res, Future[GrpcResponseMetadata]]


Encoding the metadata in the materialized value looks good.

It seems a 'clever' solution which sometimes backfires, but in this case it looks like it works out elegantly.

I agree since the metadata is in the materialized value the need for a separate invokeWithMetadata kind of disappears... unless perhaps there's opportunity for more optimization in invoke later?

Yeah, that may be a good argument for keeping the two separate methods

raboof · 2018-05-28T13:28:31Z

runtime/src/main/scala/akka/grpc/internal/AkkaGrpcGraphStage.scala

+ * @param streamingResponse Do we expect a stream of responses or does more than 1 response mean a faulty server?
+ */
+@InternalApi
+private final class AkkaNettyGrpcClientGraphStage[I, O](


…ary calls

…sts. Bonus feature - does backpressure - akka#83

…ders enabled and passing

johanandren · 2018-05-31T08:49:56Z

Should be ready for merge now! :)

raboof

Nice with the integration tests, hadn't seen those in detail yet and indeed look reasonable

johanandren · 2018-05-31T11:07:37Z

Ah, crap, I missed that it maybe failed validation, github refuses to show me what failed right now though.

raboof · 2018-05-31T11:08:11Z

No worries, I think I fixed it on master

octonato reviewed May 7, 2018

View reviewed changes

raboof reviewed May 7, 2018

View reviewed changes

johanandren force-pushed the wip-191-client-side-power-api-johanandren branch from 3feee45 to 40e678e Compare May 24, 2018 13:46

raboof mentioned this pull request May 25, 2018

Client close doesn't kill stream on server #212

Closed

ktoso approved these changes May 28, 2018

View reviewed changes

raboof approved these changes May 28, 2018

View reviewed changes

johanandren changed the title ~~[WIP] Client power user API~~ Client power user API May 28, 2018

johanandren added 14 commits May 31, 2018 11:39

Scala client API try

bd692d9

Reuse the request builder instances for simple requests

f242e64

Java side of lifted client, plus some review feedback applied.

d456c24

Docs

7af0c31

Test coverage stubs added, but needs response headers to verify

b981cad

First step towards actually providing the metadata to the user for un…

85c6f79

…ary calls

Custom graph stage to be able to access metadata for streaming reuque…

8aae712

…sts. Bonus feature - does backpressure - akka#83

Some fallout after replacing the client graphstage

1fb0a99

Broken plugin-tester code fixed

a8dfc17

Metadata through user level client API, take 1

cc69d6a

Custom, immutable, metadata types, passing end-to-end tests using hea…

b84ad14

…ders enabled and passing

Some small review things adressed

078b30b

Allow for multiple headers with the same key

8410963

Rebase import fixes

1f0c415

johanandren force-pushed the wip-191-client-side-power-api-johanandren branch from ef64ad3 to 1f0c415 Compare May 31, 2018 08:45

raboof approved these changes May 31, 2018

View reviewed changes

johanandren merged commit 92208b1 into akka:master May 31, 2018

johanandren added a commit to johanandren/akka-grpc that referenced this pull request May 31, 2018

API changes that I missed out doing in PR akka#201

c48be5e

raboof added a commit that referenced this pull request May 31, 2018

API changes that I missed out doing in PR #201 (#215)

3b0b98a

TimMoore mentioned this pull request Jun 4, 2018

Response transformation in the power-user API #229

Closed

johanandren mentioned this pull request Jul 18, 2018

Investigate how to deal with extra clients options #72

Closed

raboof mentioned this pull request Sep 3, 2018

authentication and identity on the server side #382

Open


		import scala.concurrent.Future

		// FIXME should we provide our own immutable/thread safe response metadata abstraction?

Client power user API #201

Client power user API #201

Conversation

johanandren commented May 7, 2018

johanandren commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raboof commented May 7, 2018

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanandren commented May 8, 2018

ktoso commented May 8, 2018 • edited Loading

johanandren commented May 8, 2018

octonato commented May 8, 2018

johanandren commented May 8, 2018

octonato commented May 8, 2018

TimMoore commented May 18, 2018

johanandren commented May 21, 2018

octonato commented May 23, 2018

johanandren commented May 24, 2018

octonato commented May 24, 2018 • edited Loading

octonato commented May 24, 2018

octonato commented May 24, 2018 • edited Loading

raboof commented May 24, 2018

octonato commented May 24, 2018 • edited Loading

johanandren commented May 24, 2018

octonato commented May 24, 2018

ignasi35 commented May 24, 2018 • edited Loading

johanandren commented May 24, 2018 • edited Loading

ignasi35 commented May 25, 2018

johanandren commented May 25, 2018 • edited Loading

ktoso left a comment • edited Loading

Choose a reason for hiding this comment

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanandren commented May 31, 2018

raboof left a comment

Choose a reason for hiding this comment

johanandren commented May 31, 2018

raboof commented May 31, 2018

ktoso commented May 8, 2018 •

edited

Loading

octonato commented May 24, 2018 •

edited

Loading

octonato commented May 24, 2018 •

edited

Loading

octonato commented May 24, 2018 •

edited

Loading

ignasi35 commented May 24, 2018 •

edited

Loading

johanandren commented May 24, 2018 •

edited

Loading

johanandren commented May 25, 2018 •

edited

Loading

ktoso left a comment •

edited

Loading