-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Worker Versioning #1120
Worker Versioning #1120
Changes from 20 commits
0cb488e
8ea3f41
4c84bab
1cec8d0
9754c7e
466b45d
2dc72e7
7207661
d7eadcb
2b1fa2f
5fd2848
fd82ac6
c6e41e8
6d22155
bcc09ff
51eae5c
2b77bfa
30813c7
90edaf4
04e2915
60d4ae6
f3d24e2
c6734d1
3be58e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,10 +11,10 @@ require ( | |
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 | ||
github.com/pborman/uuid v1.2.1 | ||
github.com/robfig/cron v1.2.0 | ||
github.com/stretchr/testify v1.8.2 | ||
go.temporal.io/api v1.19.1-0.20230322213042-07fb271d475b | ||
github.com/stretchr/testify v1.8.3 | ||
go.temporal.io/api v1.19.1-0.20230524162623-0e1dbb54f8e4 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to update this after api lands? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
go.uber.org/atomic v1.9.0 | ||
golang.org/x/time v0.1.0 | ||
google.golang.org/grpc v1.54.0 | ||
golang.org/x/time v0.3.0 | ||
google.golang.org/grpc v1.55.0 | ||
google.golang.org/protobuf v1.30.0 | ||
) |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,6 +177,10 @@ type ( | |
// Deprecated: WorkflowExecutionTimeout is deprecated and is never set or | ||
// used internally. | ||
WorkflowExecutionTimeout time.Duration | ||
|
||
// VersioningIntent specifies whether the continued workflow should run on a worker with a | ||
// compatible build ID or not. See VersioningIntent. | ||
VersioningIntent VersioningIntent | ||
} | ||
|
||
// UnknownExternalWorkflowExecutionError can be returned when external workflow doesn't exist | ||
|
@@ -416,14 +420,14 @@ func IsCanceledError(err error) bool { | |
// If the workflow main function returns this error then the current execution is ended and | ||
// the new execution with same workflow ID is started automatically with options | ||
// provided to this function. | ||
// ctx - use context to override any options for the new workflow like run timeout, task timeout, task queue. | ||
// if not mentioned it would use the defaults that the current workflow is using. | ||
// ctx := WithWorkflowRunTimeout(ctx, 30 * time.Minute) | ||
// ctx := WithWorkflowTaskTimeout(ctx, 5 * time.Second) | ||
// ctx := WithWorkflowTaskQueue(ctx, "example-group") | ||
// wfn - workflow function. for new execution it can be different from the currently running. | ||
// args - arguments for the new workflow. | ||
// | ||
// ctx - use context to override any options for the new workflow like run timeout, task timeout, task queue. | ||
// if not mentioned it would use the defaults that the current workflow is using. | ||
// ctx := WithWorkflowRunTimeout(ctx, 30 * time.Minute) | ||
// ctx := WithWorkflowTaskTimeout(ctx, 5 * time.Second) | ||
// ctx := WithWorkflowTaskQueue(ctx, "example-group") | ||
// wfn - workflow function. for new execution it can be different from the currently running. | ||
// args - arguments for the new workflow. | ||
Comment on lines
+424
to
+430
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I know that your formatter fixed these which is fine, but I think the fix was to just remove the space before each (it pretended two-space means indent like code). |
||
func NewContinueAsNewError(ctx Context, wfn interface{}, args ...interface{}) error { | ||
i := getWorkflowOutboundInterceptor(ctx) | ||
// Put header on context before executing | ||
|
@@ -460,6 +464,7 @@ func (wc *workflowEnvironmentInterceptor) NewContinueAsNewError( | |
WorkflowExecutionTimeout: options.WorkflowExecutionTimeout, | ||
WorkflowRunTimeout: options.WorkflowRunTimeout, | ||
WorkflowTaskTimeout: options.WorkflowTaskTimeout, | ||
VersioningIntent: options.VersioningIntent, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we keeping this in the context or adding a new constructor for the CAN error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do too but that ship has sailed, we'll just keep it consistent here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 To keeping consistent, 👎 to new constructor or overloads in general for small options like this. We messed this up by not returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Manual construction works for me. |
||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Godoc relating to this feature needs to be documented as experimental IMO.
I know it can seem unnecessary to do to each Godoc piece and instead just the entry/affecting point, but people are going to be expecting that unless otherwise marked, you're not gonna change this variable and I don't think we want to commit to API stability on any of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, thanks