Skip to content

make RampingVersionPercentageChangedTime visible #8089

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 24, 2025
Merged

make RampingVersionPercentageChangedTime visible #8089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c01610
make rampingVersionPercentageChangedTime visible
Shivs11 Jul 23, 2025
559d65c
remove print statement
Shivs11 Jul 23, 2025
430bf91
lint complaints
Shivs11 Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion service/worker/workerdeployment/workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,7 @@ func (d *WorkflowRunner) handleSetRampingVersion(ctx workflow.Context, args *dep
d.State.RoutingConfig.RampingDeploymentVersion = worker_versioning.ExternalWorkerDeploymentVersionFromStringV31(newRampingVersion)
d.State.RoutingConfig.RampingVersionPercentage = args.Percentage
d.State.RoutingConfig.RampingVersionChangedTime = rampingVersionUpdateTime
d.State.RoutingConfig.RampingVersionPercentageChangedTime = routingUpdateTime
Copy link
Member Author

@Shivs11 Shivs11 Jul 23, 2025

Choose a reason for hiding this comment

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

@dnr - pinging ya since I wanted to confirm my hunch/understanding of what will/won't break NDE;

question: Will adding this extra field to the struct RoutingConfig make our entity workflows NDE?
context: d.State.RoutingConfig is currently being stored as part of the workflow's memo via UpsertMemo calls. In other words, we are storing the state present in the routingConfig as a history event.

My initial assumption is no, this wont' cause an NDE since changing/adding inputs does not cause NDE (unless they literally trigger the addition or removal of history events and this does not)

Copy link
Contributor

@dnr dnr Jul 23, 2025

Choose a reason for hiding this comment

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

afaik, changing the contents of a memo update does not count as NDE.

potential caveat: do you ever read the memo from workflow.GetInfo().Memo for this workflow directly? it looks like no. if you did, things could get confused since the memo you read would be different from the memo you set. but if you only set here then I think it's fine.

Copy link
Member Author

@Shivs11 Shivs11 Jul 24, 2025

Choose a reason for hiding this comment

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

I also double checked some of the SDK code to see if inputs were being compared and don't think that's the case;

do you ever read the memo from workflow.GetInfo().Memo for this workflow directly
nope, the memo is only being used for List RPC calls and we are fetching the information required in those requests from here

the only other caveat I can think about it the fact that this struct, RoutingConfig, is passed as input for every CAN run. I wonder if that is a problem but I don't think so?

d.State.ConflictToken, _ = routingUpdateTime.AsTime().MarshalBinary()
d.State.LastModifierIdentity = args.Identity

Expand Down Expand Up @@ -781,7 +782,8 @@ func (d *WorkflowRunner) handleSetCurrent(ctx workflow.Context, args *deployment
d.State.RoutingConfig.RampingVersion = ""
d.State.RoutingConfig.RampingDeploymentVersion = nil
d.State.RoutingConfig.RampingVersionPercentage = 0
d.State.RoutingConfig.RampingVersionChangedTime = updateTime // since ramp was removed
d.State.RoutingConfig.RampingVersionChangedTime = updateTime // since ramp was removed
d.State.RoutingConfig.RampingVersionPercentageChangedTime = updateTime // since ramp was removed
}

// update memo
Expand Down
Loading
Loading