Add Nexus APIs to the Temporal Go SDK #89

bergundy · 2024-04-23T19:01:11Z

Part of the ongoing effort to integrate Nexus RPC into Temporal.

nexus/sdk-go.md

mfateev · 2024-04-24T00:59:30Z

nexus/sdk-go.md

+	// RegisterNexusOperation registers an operation with a worker. Panics if an operation with the same name has
+	// already been registered on this worker or if the worker has already been started. A worker will only poll for
+	// Nexus tasks if any operations are registered on it.
+	RegisterNexusOperation(op nexus.RegisterableOperation)


I recommend registering multiple Nexus endpoints with the same worker.

I agree we'll want this but should we do it now or start simple? I anticipate this being the common case.

Can you clarify? Are you saying this should accept a collection instead of just requiring users to call it multiple times?

Here's a couple of alternative's I've posted in an internal thread:

The most flexible option is to accept a nexus.Handler in WorkerOptions and have the user construct it via:

reg := &nexus.OperationRegistry{} _ := reg.Register(op1, op2) handler, _ = reg.NewHandler() w := worker.New(client, "task-queue", worker.Options{NexusHandler: handler})

^ This allows users to customize the handler completely and intercept calls.

The second option is to accept the registry:

reg := &nexus.OperationRegistry{} _ := reg.Register(op1, op2) w := worker.New(client, "task-queue", worker.Options{NexusOperationRegistry: reg})

Saves a line of code but loses some flexibility and we'll have to expose interceptors in the Go SDK.

With the first option, you get multiple services OOTB, you just need a "router" handler, which we can also add to the Nexus SDK.

Here's some more alternatives for registering multiple services:

w.RegisterNexusService("service-name", reg) // -- or -- w.RegisterNexusHandler(handler) // handle can contain a service "router" // -- or -- w.RegisterNexusHandler("service-name", handler)

Using the handler concept gives the most flexibility.

Here are some relevant definitions for reference:

https://pkg.go.dev/github.com/nexus-rpc/[email protected]/nexus#Handler
https://pkg.go.dev/github.com/nexus-rpc/[email protected]/nexus#OperationRegistry
https://pkg.go.dev/github.com/nexus-rpc/[email protected]/nexus#Operation
https://pkg.go.dev/github.com/nexus-rpc/[email protected]/nexus#RegisterableOperation

I like
w.RegisterNexusService("service-name", reg)
and
w.RegisterNexusHandler("service-name", handler)

Something that I don't like about forcing specifying the service name is that it creates duplication.
You already have to tell the server to map a service to a namespace and task queue using the incoming service registry APIs.
Specifying the service name also complicates the service renaming process.

So I'd suggest having a way to register a handler or operations without forcing specifying the service name but still supporting backing multiple services with a single worker for these (what I assume are) less common use cases.

If we want a single API, the accepting a Handler is the most flexible option.

To clarify, are we discussing accepting multiple different services? Or a single service but multiple different operations?

Multiple services on the same task queue.

I'm currently thinking we either keep w.RegisterNexusOperation assuming multiple services on the same task queue is going to be rarely used or drop this API completely to have a single registration method.

The most flexible approach is to allow registering a handler, either with w.RegisterNexusHandler or to pass in the handler in worker options.

For a single service registration would look like this:

reg := &nexus.OperationRegistry{} _ := reg.Register(op1, op2) handler, _ = reg.NewHandler() w.RegisterNexusHandler(handler)

For multiple services, we'd have:

service1Reg := &nexus.OperationRegistry{} _ = service1Reg.Register(op1, op2) service1Handler, _ = reg.NewHandler() service2Reg := &nexus.OperationRegistry{} _ = service2Reg.Register(op3, op4) service2Handler, _ = reg.NewHandler() handler := temporalnexus.NewMuxHandler() handler.RegisterService("svc1", service1Handler) handler.RegisterService("svc2", service2Handler) w.RegisterNexusHandler(handler)

I don't think we want to support registering operations without associating them to a specific service.

cretz · 2024-04-24T12:36:31Z

nexus/sdk-go.md

+
+func NewSyncOperation[I any, O any](
+	name string,
+	handler func(context.Context, client.Client, I, nexus.StartOperationOptions) (O, error),


This is not very future proof if there are Temporal-specific start options we ever may want to add. Maybe change nexus.StartOperationOptions to temporalnexus.StartOperationOptions and have the latter just embed the former as its only field?

cretz · 2024-04-24T12:52:47Z

nexus/sdk-go.md

+	name string,
+	handler func(context.Context, client.Client, I, nexus.StartOperationOptions) (O, error),


Consider a more future proof temporalnexus.SyncOperationOptions that accepts a name and a handler. That way you can add other options (some mutually exclusive). Do not overly concern yourself with type inference, Go does not have good type inference, don't let it change the API.

For example:

temporalnexus.NewSyncOperation(temporalnexus.SyncOperationOptions[string, string]{ Name: "get-status", Handler: func(ctx context.Context, c client.Client, param string, opts nexus.StartOperationOptions) (string, error) { // ... }, })

Or we could offer a shortcut which assumes ID is the param:

temporalnexus.NewSyncOperation(temporalnexus.SyncOperationOptions[string, string]{ Name: "get-status", WorkflowQuery: "some-query", })

EDIT: Can you just add a NewSyncOperationWithOptions like you did with NewWorkflowRunOperation? Arguably we could only have the options form, but I know there is an (IMO unhealthy) obsession with type inference.

I don't think we should over complicate the API with future compat concerns.
I don't even think that WorkflowQuery is an option we want here, I'd make a NewWorkflowQueryOperation instead if we want to go that direction but in the previous proposal, you were the one advocating for a single SyncOperation concept and giving users a client.

I'm open to renaming this to NewClientOperation or NewSyncClientOperation.

I don't think we should over complicate the API with future compat concerns.

I don't think it's overcomplicating. Can ignore most of my comment, but I would expect NewSyncOperationWithOptions to exist just like NewWorkflowRunOperationWithOptions does. (having said that, I wish there was only 1 call that took options always, but seems you're settled on 2)

cretz · 2024-04-24T12:59:04Z

nexus/sdk-go.md

+)
+
+opGetStatus := temporalnexus.NewSyncOperation("get-status", func(ctx context.Context, c client.Client, id string, opts nexus.StartOperationOptions) (int, error) {
+	res, err := c.QueryWorkflow(ctx, id, "", "some-query", nil)


Can you confirm for me that we decided that operations cannot have arbitrary user-defined operations on themselves and that instead the id-as-parameter pattern is how you interact w/ an existing operation (besides the built-ins like cancel)? If this pattern is so common, we can have helpers for it.

Yes, operations have a limited pre-defined interface.

cretz · 2024-04-24T13:01:40Z

nexus/sdk-go.md

+func NewWorkflowRunOperation[I, O any](
+	name string,
+	workflow func(internal.Context, I) (O, error),
+	getOptions func(context.Context, I, nexus.StartOperationOptions) (client.StartWorkflowOptions, error),


Can this param be nil?

No, you need at least a workflow ID to start a workflow.

Can you add that this param cannot be nil in the docs? Usually people in Go don't expect to have to provide a workflow ID, so this is a change.

Hmm.. right, they'll need to if they want their operation to be idempotent. It should be a deterministic function from input to ID.

nexus/sdk-go.md

cretz · 2024-04-24T13:10:58Z

nexus/sdk-go.md

+func MyHandlerWorkflowWithAlternativeInput(workflow.Context, MyWorkflowInput) (MyOutput, error)
+
+// Alternative 1 - shortest form, for workflows that have input and outputs that map 1:1 with the operation's I/O.
+opStartTransactionAlt1 := temporalnexus.NewWorkflowRunOperation(


To clarify, the only reason this exists is for people not wanting to make an options object? But we still make them make a full workflow start options callback? Did they really save much? Maybe instead you should have a NewWorkflowRunOperationOptions that accepts these fields instead of top-level overloads.

Yes, that's the reason. I believe this will be the preferred way to expose workflows as operations so I made this shorthand form.

The reason I don't like the options struct is that you have to explicitly type the type params but yes a NewWorkflowRunOperationOptions also solves that. But keep in mind that even NewWorkflowRunOperationOptions may require overloads to support handler and (in the future) a result mapper.

The reason I don't like the options struct is that you have to explicitly type the type params

I don't think this is that bad of a thing in Go and I think we're adding extra methods do to this personal dislike. But I don't have a super strong opinion just so long as we have proper options-struct-based form we can grow on all of these.

cretz · 2024-04-24T13:12:10Z

nexus/sdk-go.md

+	// RegisterNexusOperation registers an operation with a worker. Panics if an operation with the same name has
+	// already been registered on this worker or if the worker has already been started. A worker will only poll for
+	// Nexus tasks if any operations are registered on it.
+	RegisterNexusOperation(op nexus.RegisterableOperation)


Can you clarify? Are you saying this should accept a collection instead of just requiring users to call it multiple times?

nexus/sdk-go.md

cretz · 2024-04-24T19:52:44Z

nexus/sdk-go.md

+	// GetOptions must be provided when setting this option. Mutually exclusive with Handler.
+	Workflow func(workflow.Context, I) (O, error)
+	// Options for starting the workflow. Must be set if Workflow is set. Mutually exclusive with Handler.
+	GetOptions func(context.Context, I, nexus.StartOperationOptions) (client.StartWorkflowOptions, error)


For others reading, the Handler term is commonly used in Go in places like net/http.Server.Handler and the GetOptions type of name is used in places like crypto/tls.Config.GetCertificate (even though it may feel inconsistent that it's not GetHandler or OptionsGetter)

cretz

Tentatively approve. I do want to see what the results of the worker registry thing shake out to though.

bergundy · 2024-04-29T17:43:16Z

Short update: We're going to add a "service" concept that's a grouping of operations and rename the current "service" concept to "endpoint".

This change has to start from the Nexus API spec and Go SDK and then come into this proposal.
I'm putting this one on hold until we have the prerequisites in place.

mfateev · 2024-05-01T00:25:20Z

nexus/sdk-go.md

+}
+
+// Create a [NexusClient] from a service name and an endpoint name.
+func NewNexusClient(service, endpoint string) NexusClient


Consider reversing the order to endpoint, service.

bergundy · 2024-05-01T12:37:46Z

nexus/sdk-go.md

+}
+
+// Create a [NexusClient] from a service name and an endpoint name.
+func NewNexusClient(service, endpoint string) NexusClient


Also considering adding context and an options struct to future proof this API.

cretz

Approving now that registration is solved

bergundy commented Apr 23, 2024

View reviewed changes

nexus/sdk-go.md Outdated Show resolved Hide resolved

nexus/sdk-go.md Outdated Show resolved Hide resolved

Add proposal for adding Nexus APIs to the Temporal Go SDK

49b1fc6

bergundy force-pushed the nexus-on-temporal-go-sdk branch from 5dd4bad to 49b1fc6 Compare April 23, 2024 23:48

bergundy changed the title ~~Add proposal for adding Nexus APIs to the Temporal Go SDK~~ Add Nexus APIs to the Temporal Go SDK Apr 23, 2024

mfateev reviewed Apr 24, 2024

View reviewed changes

cretz reviewed Apr 24, 2024

View reviewed changes

Address some review comments

5b20f2b

cretz reviewed Apr 24, 2024

View reviewed changes

bergundy added 2 commits April 30, 2024 06:32

Add service concept

68bb78f

NexusClient

7c1b204

bergundy mentioned this pull request Apr 30, 2024

Introduce services nexus-rpc/api#5

Merged

mfateev approved these changes May 1, 2024

View reviewed changes

bergundy commented May 1, 2024

View reviewed changes

cretz approved these changes May 7, 2024

View reviewed changes

bergundy merged commit 76cf9cb into temporalio:master May 7, 2024
2 checks passed

bergundy deleted the nexus-on-temporal-go-sdk branch May 7, 2024 16:19

bergundy mentioned this pull request May 7, 2024

Minor fixes for Nexus Go SDK proposal #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Nexus APIs to the Temporal Go SDK #89

Add Nexus APIs to the Temporal Go SDK #89

bergundy commented Apr 23, 2024 •

edited

Loading

mfateev Apr 24, 2024

bergundy Apr 24, 2024

cretz Apr 24, 2024

bergundy Apr 24, 2024

mfateev Apr 24, 2024

bergundy Apr 24, 2024

cretz Apr 24, 2024

bergundy Apr 24, 2024

bergundy Apr 24, 2024

mfateev Apr 25, 2024

cretz Apr 24, 2024

cretz Apr 24, 2024

bergundy Apr 24, 2024

cretz Apr 24, 2024 •

edited

Loading

cretz Apr 24, 2024

bergundy Apr 24, 2024

cretz Apr 24, 2024

bergundy Apr 24, 2024

cretz Apr 24, 2024 •

edited

Loading

bergundy May 3, 2024

cretz Apr 24, 2024

bergundy Apr 24, 2024

cretz Apr 24, 2024

cretz Apr 24, 2024

cretz Apr 24, 2024

cretz left a comment

bergundy commented Apr 29, 2024

mfateev May 1, 2024

bergundy May 1, 2024

cretz left a comment

		name string,
		handler func(context.Context, client.Client, I, nexus.StartOperationOptions) (O, error),

Add Nexus APIs to the Temporal Go SDK #89

Add Nexus APIs to the Temporal Go SDK #89

Conversation

bergundy commented Apr 23, 2024 • edited Loading

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

cretz Apr 24, 2024 • edited Loading

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

cretz Apr 24, 2024 • edited Loading

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

cretz left a comment

Choose a reason for hiding this comment

bergundy commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

bergundy commented Apr 23, 2024 •

edited

Loading

cretz Apr 24, 2024 •

edited

Loading

cretz Apr 24, 2024 •

edited

Loading