Added metadata support to the proxy#366
Conversation
| } | ||
|
|
||
| repeated SideEffect side_effects = 4; | ||
| repeated SideEffect side_effects = 5; |
|
@jroper Tests? |
|
|
||
| // Handle a streamed in command. | ||
| // | ||
| // The first message in will contain the request metadata, including the service name and command |
There was a problem hiding this comment.
@jroper , I have been talking about this PR with @marcellanz and we have an important question, why do you say that metadata will be sent in the first message if the metadata is defined in the protocol and is contained within the message to be forwarded? I think the metadata in that case should be passed on message by message if the underlying protocols so support. Did you mean that metadata will not be forwarded by each message? That is, will subsequent messages not contain associated metadata?
There was a problem hiding this comment.
I'll clarify.
I think it should be this:
- If the underlying transport only supports per call metadata, that metadata will be on the first message, and not on the others.
- If the underlying transport supports per message metadata, each message will contain that metadata.
There was a problem hiding this comment.
Thanks @jroper
We imagined it, thank you for clarifying that. It would be nice to add this text to the PR
|
Re: tests - without a client side implementation, it's a bit hard to test meaningfully. That will come in subsequent PRs. |
pvlugter
left a comment
pvlugter
left a comment
There was a problem hiding this comment.
Not knowing the serve code or its history, this all looks fine to me.
For big changes like this (including the other related PRs), I'd be fine if it was all in one PR but broken down into commits that can be reviewed separately, with all the changes tested together.
| final def serve( | ||
| services: List[ | ||
| (ServiceDescriptor, PartialFunction[HttpRequest, Future[(List[HttpHeader], Source[ProtobufAny, NotUsed])]]) | ||
| ] |
There was a problem hiding this comment.
Funny that the type is long enough to be formatted like this. If the request handler type is used in a few places, maybe this would be better with some type aliases?
There was a problem hiding this comment.
Yeah, I think actually what would be better is a big refactor of this, not just with type aliases, but traits, so that things can be documented nicely.
| reply @ UserFunctionReply(Some(ClientAction(ClientAction.Action.Reply(Reply(_, metadata, _)), _)), | ||
| _, | ||
| _) | ||
| ) => |
There was a problem hiding this comment.
Funky match, just to get to the metadata :)
There was a problem hiding this comment.
I'm fine with changes being broken apart over multiple PRs in this case, and trust that a lot of it's been tested manually, but ideally we could have changes with tests together, in a series of commits in one PR.
I restarted the native-image test, which has been failing. We've been going over our CircleCI subscription, so we may want to disable those for PRs, and ideally have a label or something that can be added to test this.
And there's a merge conflict with master.
This adds metadata support to the proxy, and also modifies the contract in the protocol for stateless functions. This PR has been pulled out of the projections PR, in an effort to try and simplify that PR. Eventing support has been completely disabled in this PR (large parts of code are commented out), when the projections PR is merged, that will include all the necessary changes to eventing to make it work with the changes to everything else in this PR. What has been done: * Metadata support added to protocols and proxy * No user function support libraries have been modified. The metadata has been added to the protocol in a backwards compatible way, so they shouldn't need to be updated. * The one exception to this is streaming support for stateless functions in JavaScript, but that needs rewriting anyway, and I have those changes ready for a future PR, so I'm not too concerned about that.
Co-authored-by: Viktor Klang (√) <viktor.klang@gmail.com>
Co-authored-by: Peter Vlugter <pvlugter@users.noreply.github.com>
Co-authored-by: Peter Vlugter <pvlugter@users.noreply.github.com>
Co-authored-by: Peter Vlugter <pvlugter@users.noreply.github.com>
Co-authored-by: Peter Vlugter <pvlugter@users.noreply.github.com>
Co-authored-by: Peter Vlugter <pvlugter@users.noreply.github.com>
4 participants
This adds metadata support to the proxy, and also modifies the contract in the protocol for stateless functions.
This PR has been pulled out of the projections PR, in an effort to try and simplify that PR. Eventing support has been completely disabled in this PR (large parts of code are commented out), when the projections PR is merged, that will include all the necessary changes to eventing to make it work with the changes to everything else in this PR.
What has been done: