cludden / protoc-gen-go-temporal Goto Github PK

(potentially breaking) ideas in preparation of a future v1 release:

Accepted

• add support for experimental updates
• make options optional to avoid passing nils for default behavior
• move default options to parent message
• restrict rpc method option combinations to activity, activity+workflow, query, signal, update
• add NewTestClient method that accepts testsuite.WorkflowTestEnv
• add support for default search attributes mapping
• add support for documentation generation
• convert optional options to functional options to support further extension
• for all operations that support asynchronous invocation, add sync and async implementations for each (e.g. SomeWorkflow(context.Context, *SomeWorkflowRequest, ...StartWorkflowOption) (*SomeWorkflowResponse, error) and SomeWorkflowAsync(context.Context, *SomeWorkflowRequest, ...StartWorkflowOption) (SomeWorkflowRun, error)

Proposed

• support multiple proto services in a single package
• make query, signal, update methods available only from lazily initialized get method
• add multiple flavors of local activity methods (by func, by name)

Hi.

Would love to have a buf remote plugin for this. I will contribute to this if nobody else have already done it.

See: https://buf.build/docs/bsr/remote-plugins/overview

          I see u create PR, however i continue mind and detect case:

• use custom DataConverter for workflow

For ctx have func WithDataConverter() and this func not set filds in ChildWorkflowOptions - this struct not have field for it

Question: can we assume that DataConverter - option, and implement method WithDataConverter fot exlude using ctx func at all ?

example:

ctx = ctx.WithDataConverter(ctx, converter)
opts = examplev1.NewExampleV1WorkflowOptions().WithChildOptions(options)
examplev1.NewExampleV1(ctx, &examplev1.ExampleV1Request{}, opts)

->

if we assume that DataConverter - option:

opts = examplev1.NewExampleV1WorkflowOptions().WithChildOptions(options).WithDataConverter(conveter)
examplev1.NewExampleV1(ctx, &examplev1.ExampleV1Request{}, opts)

// in generated code:
type ExampleV1ChildOptions struct {
	opts                     *workflow.ChildWorkflowOptions
	// ...
	dataConverter            *converter.DataConverter
}

func ExampleV1Child(ctx workflow.Context, req *ExampleV1Request, options ...*ExampleV1ChildOptions) (*ExampleV1ChildRun, error) {
	opts := &workflow.ChildWorkflowOptions{}
	if len(options) > 0 && options[0].opts != nil {
		opts = options[0].opts
	}
	if opts.TaskQueue == "" {
		opts.TaskQueue = XnsTaskQueue
	}
	// ...
	ctx = workflow.WithChildOptions(ctx, *opts)
	if c := options[0].dataConverter; c != nil {
		ctx = workflow.WithDataConverter(ctx, *c)
	} 
        // ...
}

Originally posted by @SeJIya in #58 (comment)

Hello! I have a couple of suggestions for improvement, but they will break the current API.

1. Add synchronous and asynchronous access to Activity.
   This would eliminate the need to use Future when it's not necessary and reduce the amount of code lines.
2. Abandon the opts parameter, as the user can pass them through GetActivityOptions, which we can extract from the context. Alternatively, switch to something like configurator functions (optional) in the format of ActivityOpt.
   This would also allow not to constantly pass nils and not clutter the code.
3. Change the order of arguments: ctx, args, opts (I think the configuration should always be the last arguments).
4. Divide the LocalActivity call method into two, one by name and the other by function. This will prevent overloading one method. Example: SomeActivity3LocalByName, SomeActivity3LocalByFunc.
5. Add same access methods for Workflow?

Below I provided an example for the 3rd points:

// SomeActivity3 does some activity thing.
func SomeActivity3Local(ctx workflow.Context, fn func(context.Context, *SomeActivity3Request) (*SomeActivity3Response, error), req *SomeActivity3Request) (*SomeActivity3Response, error) {
	future := AsyncSomeActivity3Local(ctx, req, fn)
	return future.Get(ctx)
}

// SomeActivity3 does some activity thing.
func SomeActivity3(ctx workflow.Context, req *SomeActivity3Request) (*SomeActivity3Response, error) {
	future := AsyncSomeActivity3(ctx, req)
	return future.Get(ctx)
}

// SomeActivity3 does some activity thing.
func AsyncSomeActivity3(ctx workflow.Context, req *SomeActivity3Request) *SomeActivity3Future {
	opts := workflow.GetActivityOptions(ctx)
	if opts.RetryPolicy == nil {
		opts.RetryPolicy = &temporal.RetryPolicy{MaximumAttempts: int32(5)}
	}
	if opts.StartToCloseTimeout == 0 {
		opts.StartToCloseTimeout = 10000000000 // 10s
	}
	ctx = workflow.WithActivityOptions(ctx, opts)
	return &SomeActivity3Future{Future: workflow.ExecuteActivity(ctx, SomeActivity3ActivityName, req)}
}

// SomeActivity3 does some activity thing.
func AsyncSomeActivity3Local(ctx workflow.Context, req *SomeActivity3Request, fn func(context.Context, *SomeActivity3Request) (*SomeActivity3Response, error)) *SomeActivity3Future {
	opts := workflow.GetLocalActivityOptions(ctx)
	if opts.RetryPolicy == nil {
		opts.RetryPolicy = &temporal.RetryPolicy{MaximumAttempts: int32(5)}
	}
	if opts.StartToCloseTimeout == 0 {
		opts.StartToCloseTimeout = 10000000000 // 10s
	}
	ctx = workflow.WithLocalActivityOptions(ctx, opts)
	var activity any
	if fn == nil {
		activity = SomeActivity3ActivityName
	} else {
		activity = fn
	}
	return &SomeActivity3Future{Future: workflow.ExecuteLocalActivity(ctx, activity, req)}
}

In many cases, Activities are designed to be atomic units of work that can be invoked independently or as part of multiple Workflows. The current limitation necessitates the creation of a Workflow even when the use case does not require one, resulting in additional, unnecessary code which does not serve any functional purpose for the intended standalone use of Activities.

According to the documentation, setting the task_queue parameter is supposed to "Override default task queue for activity". However, this behavior does not seem to be working as expected. When specifying a task_queue, the system does not override the default task queue.

Override default options (ex. activity options) need using .WithActivityOptions method because options included in context will be rewritten with define in proto (context options ignoring if have default - https://github.com/cludden/protoc-gen-go-temporal/blob/main/gen/example/v1/example_temporal.pb.go#L857) . .WithActivityOptions - method overide all options, not have varinat chnaging one.

Before this codegen we using options in ctx for execute activities/child workflow as:

// w.baseActivityOptions = workflow.ActivityOptions{
// 	HeartbeatTimeout:    cfg.TemporalConfig.ActivityHeartbeatTimeout, // configurate from ENV/config
// 	WaitForCancellation: true,
// },
func (w *WorkflowV1) Example(ctx workflow.Context) (werr error) {
    activityCtx := workflow.WithActivityOptions(ctx, w.baseActivityOptions)
    
    deleteCtx := workflow.WithStartToCloseTimeout(activityCtx, 10 * time.Secund)
    // execute activity 
    publishCtx := workflow.WithStartToCloseTimeout(activityCtx, 15 * time.Secund)
    publishCtx = workflow.WithRetryPolicy(publishCtx, temporal.RetryPolicy{
	 MaximumAttempts: 3
    })
    // execute activity 

for now if based from generated code and need change one default options already defined in proto:

func (w *WorkflowV1) Example(ctx workflow.Context) (werr error) {
    ao := w.baseActivityOptions
    ao.StartToCloseTimeout = 10 * time.Secund
    opts = examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao)
    examplev1.NewExampleV1(ctx, *examplev1.ExampleV1Request{}, opts)

My idea/concept for adding all methods for options activity & workflow (https://github.com/temporalio/sdk-go/blob/master/workflow/activity_options.go#L59 - for activity example):

func (w *WorkflowV1) Example(ctx workflow.Context) (werr error) {
	opts = examplev1.NewExampleV1ActivityOptions().WithActivityOptions(w.baseActivityOptions).WithStartToCloseTimeout(10 * time.Second)
	// or
	opts = examplev1.NewExampleV1ActivityOptions().WithStartToCloseTimeout(10 * time.Second)
	examplev1.NewExampleV1(ctx, *examplev1.ExampleV1Request{}, opts)

generated code after:

// NewExampleV1ActivityOptions sets default ActivityOptions
func NewExampleV1ActivityOptions() *ExampleV1ActivityOptions{
	return &ExampleV1ActivityOptions{
+		opts: &workflow.ActivityOptions{},
	}
}

// WithActivityOptions sets default ActivityOptions
func (opts *ExampleV1ActivityOptions) WithActivityOptions(options workflow.ActivityOptions) *DeleteFileV1ActivityOptions {
	opts.opts = &options
	return opts
}

+func (opts *ExampleV1ActivityOptions) WithStartToCloseTimeout(d time.Duration) *DeleteFileV1ActivityOptions {
+	opts.opts.StartToCloseTimeout = d
+	return opts
+}

// and more methods...

I need your opinion for this concept.

The Validate field in UpdateOptions clashes with the Validate method generated by the buf.build/envoyproxy/protoc-gen-validate plugin.

Since envoyproxy/protoc-gen-validate is a very popular plugin, I think it'd be good to rename that field.