Skip to content
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

[Feature] Versioned Workflows #72

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

antstorm
Copy link
Contributor

A new feature that allows introducing breaking changes by swapping workflow versions without incurring non-determinism. The latest version is determined at the start_workflow and passed as a header.

@antstorm antstorm changed the title [WIP] Versioned Workflows [Feature] Versioned Workflows Feb 18, 2022
@antstorm antstorm marked this pull request as ready for review February 18, 2022 15:24
@antstorm antstorm requested a review from DeRauk February 18, 2022 15:24
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
version_class.timeouts || main_class.timeouts
end

def headers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you change the version of a cron job? Would you have to resubmit the job after updating the workflow so that the header gets sent to Cadence and persisted across future workflow starts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting! Yes, you'd have to resubmit it because all the values (along with the input arguments) are passed when you first schedule it

@DeRauk
Copy link
Contributor

DeRauk commented Feb 23, 2022

This is cool!

end
end

class MyWorkflowV2 < Cadence::Workflow

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny, we're also working on versioning.
My initial take: It's gonna be pretty hard to read PRs that copy all the code. Also this will make the git blame look like all the code is new.
We use inline version checks (version_at_least?) and we memoize the version to make it sticky. A bit of a hack, but we use

temporal_context.has_release?("version-#{version}")

where version is a number. We call this at the beginning of each workflow to memoize it.
We're adopting ElasticSearch and I think we need to implement https://github.com/coinbase/temporal-ruby/blob/master/lib/temporal/workflow/state_manager.rb#L285 so we can get the version into ES as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting… so if I understand correctly you're doing something very similar — you are picking the latest version at the start of each execution and then adding conditionals to the workflow code to segregate different version. Am I getting it right?

Btw, I don't think you need to memoize the version yourself because has_release? does it for you — after it was called ones it will return the same value for all subsequent calls.

We were following the same approach using the has_release? explicitly, but found it to be quite noisy.

Maybe the optimal solution is the combination of the two — version is submitted explicitly via headers and then can be checked inline with version_at_least?. It allows for feature flag rollouts as well as concise PR diffs.

I like the idea of adding ES support and tagging versions for searchability 👍

version 1, MyWorkflowV1
version 2, MyWorkflowV2
version_picker do |_latest_version|
if my_feature_flag?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool. We're actually considering forcing our engineers to use a feature flag to avoid bouncing back and forth between code versions during deploys. I know it's harder in an SDK designed for many companies, but I wonder if you could nudge this toward being more encouraged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to it and think it's a great practise. Do you have any specific ideas on how you'd force it? Removing the default version_picker implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants