More idiomatic play integration #420 #534
Conversation
|
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
|
Refs #420 |
|
OK TO TEST |
| public Function1<RequestHeader, EssentialAction> createHandler(String serviceName, Materializer mat) { | ||
| return new Function1<RequestHeader, EssentialAction>() { | ||
| public EssentialAction apply(RequestHeader reqOuter) { | ||
| Function<HttpRequest, CompletionStage<HttpResponse>> handler = @{serviceName}HandlerFactory.create(Abstract@{serviceName}Router.this, serviceName, mat, eHandler, system); |
There was a problem hiding this comment.
If I'm reading this correctly, this would now be called for every request. It looks like that would not be necessary, and it could be created once outside of the function. Would that work?
There was a problem hiding this comment.
No, this is only called once during initialization. See here.
It shouldn't be public though, will change that.
There was a problem hiding this comment.
Sure, createHandler is only called once, but line 66 is inside the function that is returned by createHandler, and that is called for every request - unless I'm misreading this somehow, it could be moved to between line 64 to only create the handler once? (haven't tried though)
There was a problem hiding this comment.
Ah right, I was looking at line 64. Will fix.
|
|
||
| def createStreamingAction(handler: HttpRequest => Future[HttpResponse])(implicit ec: ExecutionContext): EssentialAction = | ||
| actionBuilder.async(streamBodyParser) { req => | ||
| handler(playToAkkaRequestStream(req)).map(akkaToPlayResp) |
There was a problem hiding this comment.
As discussed in #420 (comment), this 'additional roundtrip' is of course more or less unavoidable if you want to take into account the changes to the Play request, but does come at some performance cost.
How much performance cost, and whether that cost is acceptable, of course highly depends on the application and should be measured/considered by the user.
I think we should add some documentation to this option, and explain clearly that the trade-off here: this option allows you to use your existing Play action composition approach, at the expense of some performance (which is why it is not the default). Later, eventually, we could also document alternate ways to achieve cross-cutting concerns there.
There was a problem hiding this comment.
Currently there's no mention of the Play integration in the docs. Where do you suggest documenting this?
There was a problem hiding this comment.
Ah, the Play gRPC documentation has already moved to https://github.com/playframework/play-grpc - I guess we will have to move the documentation for this feature to a separate PR over there.
There was a problem hiding this comment.
Ok, I'll work on that before long, but you can go ahead and merge this right?
|
Hi @marcospereira , when do you think you can review this? Want to get closure on this odyssey. |
|
Hi @raboof , nothing's happening from your added reviewer and I'd like this to get merged and done with. Please advise. |
|
This is now good to go - thanks for another great contribution! 🎉 |
|
Yay! Thanks @raboof . |
Continuation of PR #447, using a new branch with single composite commit.