-
Notifications
You must be signed in to change notification settings - Fork 124
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
feat: http transcoding grpc #1583
Conversation
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
d70d6a6
to
15b6a93
Compare
6b3cac4
to
86d2360
Compare
I've only implement scala mode. I'm not quite familiar with Java. Also, I think we can make grpc client works for json format http request. That will make akka-grpc usable even we only use protobuf to describe service protocols. |
} | ||
|
||
val httpApiHandlers = (List.empty[PartialFunction[model.HttpRequest, scala.concurrent.Future[model.HttpResponse]]] @for(method <- service.methods if !method.inputStreaming){ ++ @{method.name}Handler}) | ||
.foldLeft(PartialFunction.empty[model.HttpRequest, scala.concurrent.Future[model.HttpResponse]])((acc, p) => acc.orElse(p)) |
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's very likely that you'll want to use a different method of partial function composition (I remember that I had to do something more custom to make sure that it was well-behaved in the face of many endpoints when I did the Cloudstate version). Otherwise you'll have to do a match linearly against all possible cases for each request.
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, you are right. I've changed my implementation. I can get grpc handler and use it, so what I need to do is transforming json http request to grpc request and handle it with grpc handler, then transform grpc response to json http response. Not too efficient, but it might work.
86d2360
to
1875e3b
Compare
At least one commit author ([email protected]) is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
1875e3b
to
70749bf
Compare
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 is already much less intrusive, which is great! I wonder if we could take that even further?
Function.unlift((req: model.HttpRequest) => req.uri.path match { | ||
|
||
|
||
val httpHandler = akka.grpc.HttpApi.serve(@{service.name}.descriptor, handle _) |
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 effectively elevates HttpApi.serve
to stable API: if a project uses code generated by this version, and updates the Akka gRPC runtime library, we'll have to make sure these new versions of the runtime library still provide that serve
function with the same signature and behavior. I'm not sure I'm comfortable with that yet.
I wonder if it would be possible to use this HttpApi
from the main client code, so the user can optionally enable it, and we can mark it ApiMayChange
to leave room for evolving the API further later?
case model.Uri.Path.Slash(model.Uri.Path.Segment(`prefix`, model.Uri.Path.Slash(model.Uri.Path.Segment(method, model.Uri.Path.Empty)))) => | ||
Some(handle(spi.onRequest(prefix, method, req), method)) | ||
case _ => | ||
None | ||
}) | ||
httpHandler orElse grpcHandler |
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 also seems potentially tricky: some high-throughput use cases might only care about the gRPC protocol, and hard-coding the HTTP transcoding support here might impact that hot path.
Closing this since continued in #1834 |
References #1461.
Most code of this commit are from file https://github.com/cloudstateio/cloudstate/blob/master/proxy/core/src/main/scala/io/cloudstate/proxy/HttpApi.scala
Just do some change to make it may work in akka-grpc.