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.

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

Jump to bottom

Pass versioning info when adding Matching tasks #6879

Merged
merged 10 commits into from
Nov 25, 2024
Merged

Pass versioning info when adding Matching tasks #6879

merged 10 commits into from
Nov 25, 2024

Conversation

ShahabT
Copy link
Collaborator

@ShahabT ShahabT commented Nov 22, 2024

What changed?

Workflow versioning info needs to be passed to tasks sent to Matching so they can be places in right queues.

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

@ShahabT ShahabT requested review from dnr and alexshtin November 22, 2024 23:41
@ShahabT ShahabT requested a review from a team as a code owner November 22, 2024 23:41
alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/api/queryworkflow/api.go Outdated
@@ -364,3 +366,16 @@ func queryDirectlyThroughMatching(
QueryRejected: matchingResp.GetQueryRejected(),
}}, err
}

func GetWorkflowVersioningInfoMatchingTask(msResp *historyservice.GetMutableStateResponse) *deploymentpb.WorkflowVersioningInfo {
Copy link
Member

Choose a reason for hiding this comment

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

Everything besides Invoke in api dir must be unexported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed this.

alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/api/updateworkflow/api.go Outdated
@@ -216,6 +218,7 @@ func (u *Updater) ApplyRequest(
ms.GetMostRecentWorkerVersionStamp(),
ms.HasCompletedAnyWorkflowTask(),
)
u.workflowVersioningInfo = workflow.GetWorkflowVersioningInfoMatchingTask(ms)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
u.workflowVersioningInfo = workflow.GetWorkflowVersioningInfoMatchingTask(ms)
u.workflowVersioningInfo = common.CloneProto(workflow.GetWorkflowVersioningInfoMatchingTask(ms))

Copy link
Member

Choose a reason for hiding this comment

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

and u.directive above too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

workflowVersioningInfo is removed and u.directive does not come from MS, it's created fresh.

alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/api/updateworkflow/api.go
@@ -231,15 +234,15 @@ func (u *Updater) OnSuccess(
// Speculative WFT was created and needs to be added directly to matching w/o transfer task.
// TODO (alex): This code is copied from transferQueueActiveTaskExecutor.processWorkflowTask.
// Helper function needs to be extracted to avoid code duplication.
err := u.addWorkflowTaskToMatching(ctx, u.wfKey, u.taskQueue, u.scheduledEventID, u.scheduleToStartTimeout, u.directive)
err := u.addWorkflowTaskToMatching(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I can't recall why we pass all these Updater fields to Updater method. @stephanos, is there a specific reason? Because this change makes perfect sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't see one. I must have transferred them from vars to fields and missed that obvious refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here: e819c3a

alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/api/updateworkflow/api.go Outdated
Comment on lines 71 to 72
directive *taskqueuespb.TaskVersionDirective
workflowVersioningInfo *deploymentpb.WorkflowVersioningInfo
Copy link
Member

Choose a reason for hiding this comment

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

can you move both of this under line 80. They need to be under this WARNING comment below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

workflowVersioningInfo is removed and u.directive does not come from MS, it's created fresh.

alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/transfer_queue_active_task_executor.go Outdated
@@ -216,13 +216,14 @@ func (t *transferQueueActiveTaskExecutor) processActivityTask(

timeout := timestamp.DurationValue(ai.ScheduleToStartTimeout)
directive := MakeDirectiveForActivityTask(mutableState, ai)
wfVersioningInfo := workflow.GetWorkflowVersioningInfoMatchingTask(mutableState)
Copy link
Member

Choose a reason for hiding this comment

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

see this scare NOTE below.

Suggested change
wfVersioningInfo := workflow.GetWorkflowVersioningInfoMatchingTask(mutableState)
wfVersioningInfo := common.CloneProto(workflow.GetWorkflowVersioningInfoMatchingTask(mutableState))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed this

alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/transfer_queue_active_task_executor.go Outdated
@@ -265,6 +266,8 @@ func (t *transferQueueActiveTaskExecutor) processWorkflowTask(

directive := MakeDirectiveForWorkflowTask(mutableState)

wfVersioningInfo := workflow.GetWorkflowVersioningInfoMatchingTask(mutableState)
Copy link
Member

Choose a reason for hiding this comment

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

Oh boy, it is everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not anymore.

alexshtin
alexshtin reviewed Nov 23, 2024
View reviewed changes
service/history/workflow/mutable_state_impl.go
Comment on lines 6744 to +6745
func (ms *MutableStateImpl) GetEffectiveDeployment() *deploymentpb.Deployment {
versioningInfo := ms.GetExecutionInfo().GetVersioningInfo()
if versioningInfo == nil {
return nil
} else if transition := versioningInfo.GetDeploymentTransition(); transition != nil {
return transition.GetDeployment()
} else if override := versioningInfo.GetVersioningOverride(); override != nil &&
override.GetBehavior() == enumspb.VERSIONING_BEHAVIOR_PINNED {
return override.GetDeployment()
} else if ms.GetEffectiveVersioningBehavior() != enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED {
return versioningInfo.GetDeployment()
}
return nil
return GetEffectiveDeployment(ms.GetExecutionInfo().GetVersioningInfo())
Copy link
Member

Choose a reason for hiding this comment

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

I don't like these names but I can't come up with something better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can change later if you come up with something better :)

Base automatically changed from shahab/v3-internal-protos to versioning-3 November 23, 2024 02:31
@ShahabT
Merge remote-tracking branch 'origin/versioning-3' into shahab/transf…
544e62f 
…er-versioning-info

# Conflicts:
#	api/deployment/v1/message.pb.go
#	api/historyservice/v1/request_response.pb.go
#	api/persistence/v1/tasks.pb.go
#	api/taskqueue/v1/message.pb.go
#	proto/internal/temporal/server/api/deployment/v1/message.proto
#	proto/internal/temporal/server/api/historyservice/v1/request_response.proto
@ShahabT
merge versioning-3
3a2c03c
@ShahabT ShahabT force-pushed the shahab/transfer-versioning-info branch from 4b131f6 to 3a2c03c Compare November 23, 2024 05:58
alexshtin
alexshtin approved these changes Nov 25, 2024
View reviewed changes
proto/internal/temporal/server/api/persistence/v1/executions.proto Outdated
@@ -258,6 +258,9 @@ message WorkflowExecutionInfo {

// When present, it means the workflow execution is versioned, or is transitioning from
// unversioned workers to versioned ones.
// Note: Deployment objects inside versioning info are immutable, never change their fields.
// (-- api-linter: core::0203::immutable=disabled
// aip.dev/not-precedent: field_behavior annotation not available in our gogo fork --)
Copy link
Member

Choose a reason for hiding this comment

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

Where did you copy this from? We are not on gogo fork anymore :-)

@ShahabT ShahabT merged commit 848d76c into versioning-3 Nov 25, 2024
8 checks passed
@ShahabT ShahabT deleted the shahab/transfer-versioning-info branch November 25, 2024 19:57
dnr pushed a commit that referenced this pull request Nov 26, 2024
@ShahabT @dnr
Pass versioning info when adding Matching tasks (#6879)
c8de247 
## What changed?
<!-- Describe what has changed in this PR -->
Workflow versioning info needs to be passed to tasks sent to Matching so
they can be places in right queues.

## Why?
<!-- Tell your future self why have you made these changes -->

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
dnr pushed a commit that referenced this pull request Nov 26, 2024
@ShahabT @dnr
Pass versioning info when adding Matching tasks (#6879)
679fb19 
## What changed?
<!-- Describe what has changed in this PR -->
Workflow versioning info needs to be passed to tasks sent to Matching so
they can be places in right queues.

## Why?
<!-- Tell your future self why have you made these changes -->

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanos stephanos stephanos left review comments

@alexshtin alexshtin alexshtin approved these changes

@dnr dnr Awaiting requested review from dnr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShahabT @stephanos @alexshtin