-
Notifications
You must be signed in to change notification settings - Fork 270
Stop sending deployment info for session per-worker task queue #2088
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
Stop sending deployment info for session per-worker task queue #2088
Conversation
| params.TaskQueue = sessionEnvironment.GetResourceSpecificTaskqueue() | ||
| // For the resource specific task queue, we don't need to include deployment options | ||
| // Save them to restore later | ||
| deployments := params.DeploymentOptions |
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.
Only disable versioning instead of removing the whole thing? better to keep the deployment name and build id if user has given it (to keep in the events).
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.
Does it make any difference for an activity only task queue? I don't see any difference in the history.
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.
we might add the deployment options in activity events later
eddebc9 to
5fc9d01
Compare
| } | ||
|
|
||
| func (ts *WorkerDeploymentTestSuite) TestBuildIDWithSession() { | ||
|
|
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.
Do we want to gate this? All other tests here have this req
if os.Getenv("DISABLE_SERVER_1_27_TESTS") != "" {
ts.T().Skip("temporal server 1.27+ required")
}
| wfHandle, err := ts.client.ExecuteWorkflow(ctx, ts.startWorkflowOptions("evolving-wf-1"), "SessionBuildIDWorkflow") | ||
| ts.NoError(err) | ||
|
|
||
| ts.NoError(wfHandle.Get(ctx, nil)) |
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.
What is this actually verifying? This seems to pass on master without your fix
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.
Should we be asserting that buildId and versioning settings for the session activity worker is properly set?
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.
Yeah I am verifying it still works and I didn't break anything.
5fc9d01 to
6ef51b0
Compare
Stop sending deployment info for session per-worker task queue
Note
Temporarily disables deployment/versioning for the session activity worker on the resource-specific task queue (restored for creation worker) and adds a session build ID test.
newSessionWorker, saveparams.DeploymentOptions/params.UseBuildIDForVersioning, setDeploymentOptions.UseVersioning=falseandUseBuildIDForVersioning=falsefor the session activity worker on the resource-specific task queue, then restore for the creation worker.workerOverridesconstruction.TestBuildIDWithSessionintest/worker_deployment_test.goto validate running a session-enabled worker with deployment versioning; registers session workflow and activity usingactivity.RegisterOptionsand verifies workflow completion.Written by Cursor Bugbot for commit 6ef51b0. This will update automatically on new commits. Configure here.