-
Notifications
You must be signed in to change notification settings - Fork 285
Versioning/upgrade on can #2171
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
base: master
Are you sure you want to change the base?
Conversation
cretz
left a comment
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.
Nothing blocking, unsure if @yuandrew also wants to review
|
|
||
| // InitialVersioningBehavior specifies the versioning behavior that the first task of the new run should use. | ||
| // For example, choose to AutoUpgrade on continue-as-new instead of inheriting the pinned version of the previous run. | ||
| InitialVersioningBehavior ContinueAsNewVersioningBehavior |
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.
This needs to be marked experimental IMO
| // GetContinueAsNewSuggestedReasons returns a list of reasons why continue as new is suggested, | ||
| // if the server is configured to suggest continue as new and it is suggested. | ||
| // This value may change throughout the life of the workflow. | ||
| func (wInfo *WorkflowInfo) GetContinueAsNewSuggestedReasons() []ContinueAsNewSuggestedReason { |
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.
This needs to be marked experimental IMO
test/worker_deployment_test.go
Outdated
| return false | ||
| } | ||
| return false | ||
| }, 10*time.Second, 300*time.Millisecond) |
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.
Would like to see all of the worker deployment "eventually" stuff reduces to max timeout 5s and interval 100ms if we can, but not blocking
internal/internal_event_handlers.go
Outdated
| func convertContinueAsNewSuggestedReasonsFromProto( | ||
| reasons []enumspb.SuggestContinueAsNewReason, | ||
| ) []ContinueAsNewSuggestedReason { | ||
| var converted []ContinueAsNewSuggestedReason |
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.
Pedantic, but you should pre-set the capacity here if you know it
|
I'd like to take a look, will look later today or early tmr |
yuandrew
left a comment
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.
one small nit, but LGTM otherwise. Agree with all of Chad's points on marking experimental and decreasing eventually() bounds
internal/error.go
Outdated
| if options.RetryPolicy != nil { | ||
| continueAsNewErr.RetryPolicy = options.RetryPolicy | ||
| } | ||
| if options.InitialVersioningBehavior != ContinueAsNewVersioningBehaviorUnspecified { |
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.
nit: I don't think we need this if check, since the default of both continueAsNewErr.InitialVersioningBehavior and options.InitialVersioningBehavior are the same
carlydf
left a comment
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.
approved! not sure why tests are failing, but lmk if you want any help fixing them
We're testing on a version of CLI ( |
What was changed
Expose
InitialVersioningBehavioronContinueAsNewErrorContinueAsNewErrorOptionsthis specifies the versioning behavior that the first task of the new run should use.
Expose
continueAsNewSuggestedReasonsonWorkflowInfo, which describes the reason(s) that the CAN is suggested.Why?
Provides workflow version control on CAN boundaries. This change in particular allows users to auto-upgrade on CAN.
Part of: Expose upgrade on continue as new in SDKs features#738
How was this tested:
Integration tests
Any docs updates needed?
Yes.