Nexus callbacks, tasks, and incoming service registry #352
Conversation
There was a problem hiding this comment.
I assume Nexus operation events come later?
There was a problem hiding this comment.
Yeah, we're not quite there yet in terms of stable implementation.
I plan to open a PR with those that will sit around for a while in a feature branch very soon.
| // Callback URL. | ||
| // (-- api-linter: core::0140::uri=disabled | ||
| // aip.dev/not-precedent: Not following this guideline. --) | ||
| string url = 1; |
There was a problem hiding this comment.
I forget whether this should be a URL or a service name. Aren't URLs configured as part of services and service names used in place of URLs everywhere?
There was a problem hiding this comment.
I know it's limited feature set only for now, but one might expect there to be a lot of config that one would want to set for callback URLs. Is that on the future radar?
There was a problem hiding this comment.
The config is provided via dynamic config per address.
We may allow setting headers and more customization at callback attachment time, we'll evaluate as needed.
| } | ||
|
|
||
| message HandlerError { | ||
| // See https://github.com/nexus-rpc/api/blob/main/SPEC.md#predefined-handler-errors. |
There was a problem hiding this comment.
nit: should this be on the message instead of the field similar to the Failure message, or is that intentional?
There was a problem hiding this comment.
The comment is referring to the error type.
| } | ||
|
|
||
| // A request to start an operation. | ||
| message StartOperationRequest { |
There was a problem hiding this comment.
Do you have a link to the docs for this one as well?
There was a problem hiding this comment.
I don't know if it's strictly needed. I didn't feel like it was necessary.
| // A Nexus request. | ||
| message Request { | ||
| // Headers extracted from the original request in the Temporal frontend. | ||
| // When using Nexus over HTTP, this includes the request's HTTP headers ignoring multiple values. |
There was a problem hiding this comment.
ignoring multiple values.
Which values? I assume something like Temporal-but-not-Nexus-specific headers?
There was a problem hiding this comment.
HTTP headers can have multiple values, we only take the first one.
| @@ -152,3 +153,32 @@ message NewWorkflowExecutionInfo { | |||
| temporal.api.common.v1.Header header = 13; | |||
| } | |||
|
|
|||
| message CallbackInfo { | |||
There was a problem hiding this comment.
I think a quick note about what this message is could be useful. There's a few different CallbackXXX types, and there should be some disambiguation. Like, how does this differ from temporal.api.common.v1.Callback?
| // A unique identifier for this task. | ||
| bytes task_token = 1; |
There was a problem hiding this comment.
Is this intentionally opaque? If so, I think it'd be nice to state what its purpose is, i.e. is it for idempotency?
There was a problem hiding this comment.
It's common in all of our APIs. Yes, it's intentionally opaque.
It's for correlating a poll response with a completion request.
I'll add it to the docstring.
| // The identity of the client who initiated this request. | ||
| string identity = 2; |
There was a problem hiding this comment.
What is this used for? I think it'd be nice to say because there's a lot of different "identity" fields throughout our APIs, and I believe they don't always have the same behavior or purpose.
There was a problem hiding this comment.
It's always to the caller identity. It's likely useful only for debugging but we don't persist it, log it it or send it back to the caller.
This PR takes the work from the
nexusfeature branch and merges it intomaster.All of this has already been reviewed in previous PRs into the
nexusbranch but now that we're making progress with the implementation and feel confident in this API, it's time to merge.