Worker Deployment Versioning #1679
Conversation
There was a problem hiding this comment.
Didn't really look through the core bridge stuff, maybe worth coordinating on #1638.
For your workflow test files deployment-versioning-no-annotations... there's a workflows directory in test/src that might make more sense for them to be located, instead of directly in test/src.
Just want to re-affirm how this works:
- on the worker, we have the worker's configured deployment version, and the default versioning behaviour for all its workflows
- on the workflow, we can override this versioning behaviour with the newly added workflow definition options (it's a little funky, we're making use of JS's quirk that a function is an object, so a workflow with definition options is a function with properties, as already discussed)
- workflow versioning behaviour is read into the activator (which uses it solely when concluding the activation) and workflow info
Tests were helpful, though I think you probably want someone whose more familiar with the versioning behaviour to take a look
|
|
||
| // Wait for worker 1 to be visible and set as current version | ||
| const describeResp1 = await waitUntilWorkerDeploymentVisible(client, w1DeploymentVersion); | ||
| await setCurrentDeploymentVersion(client, describeResp1.conflictToken, w1DeploymentVersion); |
There was a problem hiding this comment.
I'm not understanding these setCurrentDeploymentVersion calls (this and the subsequent calls in this test), we are setting it's deployment version to it's current deployment version? What does this achieve?
There was a problem hiding this comment.
What is "it" in that sentence? What this does is tell the server that the current version for some named deployment like deployment is a specific version like deployment.1.0
There was a problem hiding this comment.
"it" meant worker
oh, so this call is purely for server's awareness
|
|
||
| const test = makeTestFunction({ workflowsPath: __filename }); | ||
|
|
||
| test('Worker deployment based versioning', async (t) => { |
There was a problem hiding this comment.
IIUC, there isn't any actual upgrading going on in this test? This test is checking that workflows with different versioning behaviours can run on workers with different deployment options?
There was a problem hiding this comment.
It definitely does. The wf1 workflow starts on v1 and ends on v3
packages/workflow/src/internals.ts
Outdated
| } from '@temporalio/common'; | ||
| import { | ||
| decodeSearchAttributes, | ||
| decodeTypedSearchAttributes, | ||
| } from '@temporalio/common/lib/converter/payload-search-attributes'; | ||
| import { composeInterceptors } from '@temporalio/common/lib/interceptors'; | ||
| import { makeProtoEnumConverters } from '@temporalio/common/lib/internal-workflow'; | ||
| import type { coresdk, temporal } from '@temporalio/proto'; | ||
| import { coresdk, temporal } from '@temporalio/proto'; |
There was a problem hiding this comment.
Never do a regular import to proto from Workflow code (that's a strong no). And as a general rule, avoid using anything from proto in user-facing APIs (workflow code or not).
In this case, you will need to create a pure JS mirror of the enum. Search for makeProtoEnumConverters in the project for examples you can copy from. That JS type should be defined in the common package, as you will want it to also be accessible in non-workflow context.
There was a problem hiding this comment.
Thanks for the pointers!
| @@ -416,6 +418,8 @@ export class Activator implements ActivationHandler { | |||
|
|
|||
| public readonly registeredActivityNames: Set<string>; | |||
|
|
|||
| public versioningBehavior?: VersioningBehavior; | |||
There was a problem hiding this comment.
How do dynamic workflows pick their versioning behaviour per type?
There was a problem hiding this comment.
They can do it like is shown here: https://github.com./temporalio/sdk-typescript/pull/1679/files#diff-5ae1c5b6523b9493d1f852e9d303621ffdc8f817f1b80f38f572008572aec6feR14
In Python Chad didn't want to have a dynamic getter like we added in Java unless someone asks, which I can get behind, so I figured we'd do the same here.
There was a problem hiding this comment.
Not sure it makes sense to cut features from some SDKs, but not other
There was a problem hiding this comment.
I think the getter is a touch error prone. We could remove it from Java too, if no one ends up needing it in these other places. I didn't want to hold up being able to finish the Core ones over that, given these can get changed later anyway. Chad is who really cared: temporalio/sdk-python#821 (comment)
There was a problem hiding this comment.
I think the getter is a basic requirement since versioning a per workflow type option per the design doc. Dynamic workflows implement multiple workflow types. I am just not sure why other SDKs users have to have reduced functionality.
There was a problem hiding this comment.
I'm not the one who didn't want to add it 🤷♂️ . I agree with the principle. In practice, I suspect most people doing dynamic workflows would be fine with one behavior. It's hard to say. I'm fine either way, I think Chad is who you have to convince.
Sushisource
force-pushed
the
worker-deployment-versioning
branch
3 times, most recently
from
22ea480 to
a120422
Compare
Sushisource
force-pushed
the
worker-deployment-versioning
branch
from
a120422 to
e59e2d0
Compare
What was changed
Added the annotations and options for worker-deployment-based versioning
Why?
All aboard the versioning train
Checklist
Closes [Feature Request] Support New Worker Versioning API #1659
How was this tested:
Added tests
Any docs updates needed?