Document safe Workflow patch retirement - #257
Conversation
77a2a7b to
95da572
Compare
|
Thank you for this @robzienert ! Sorry about the delay. getting this reviewed ASAP. |
Sushisource
left a comment
There was a problem hiding this comment.
There's quite a lot of llm-speak in here that I think is adding more tokens than value.
I think 90% of the value here would be captured by adding a line about the difference between open and retained workflows and what that means for you if you care about querying closed workflows. There's maybe also some good bits around using the search attribute correctly.
Most of the rest of it seems to be verbose-ifying information that's already there.
I think we could cut this down to a few lines added to the core file and that'd be better.
| - Wait until executions that can return the retired version have left retention before removing that branch or raising the minimum supported version. | ||
| - Keep the Workflow's deprecation/version marker at the same deterministic location for history compatibility. |
There was a problem hiding this comment.
It's worth mentioning you only need to wait until they've left retention if you query them after they've completed, which is somewhat rare. If you know you don't do that, then you can remove the code when they've all completed, but before they've been archived/deleted.
| - After all deprecated workflows complete | ||
| - Remove patch entirely, only new code remains | ||
| - Removing the final marker is a separate operation with patching-API-specific rules beyond branch retirement. | ||
| - Retire the patch/change identifier permanently when the patching API requires it; do not reuse an identifier whose first marker call was removed. |
There was a problem hiding this comment.
This seems unhelpful. It's basically saying "you can remove the marker when you should", whereas the old text tried to tell you when "should" is. Some combination of old/new would be appropriate here.
| ## Best Practices | ||
|
|
||
| 1. **Check for open executions** before removing old code | ||
| 1. **Wait for affected executions to leave retention** before removing old code |
There was a problem hiding this comment.
Same comment about the difference between open & retention
| # Find pre-patch workflows | ||
| temporal workflow list --query \ | ||
| 'WorkflowType = "OrderWorkflow" AND TemporalChangeVersion IS NULL' | ||
| Worker Deployment versions use a separate Search Attribute: |
There was a problem hiding this comment.
This seems to have dropped a bunch of context about how to do queries with TemporalChangeVersion?
The previous paragraph refers to it without explaining what it is
| **Step 2: Deprecate the Patch** | ||
|
|
||
| Once all pre-patch Workflow Executions have completed: | ||
| After all pre-patch Workflow Executions have left retention: |
| **Step 3: Remove the Patch** | ||
|
|
||
| After all workflows with the deprecated patch marker have completed, remove the `DeprecatePatch()` call entirely: | ||
| In a later deployment, after the deprecation rollout is complete and the pre-patch Workflow Executions have left retention, remove the `DeprecatePatch()` call entirely. A deprecated marker is intentionally safe to omit during replay, so marker-bearing executions do not require another retention wait: |
There was a problem hiding this comment.
This last sentence is hard to parse. I think it means something like "If a workflow's history contains a deprecated marker, it can safely be replayed by code without any of the old code or the patch call."
| `TemporalChangeVersion IS NULL` is reliable only when no other patch marker could make the attribute non-null. For multiple or optional patch sites, inspect all retained executions of the Workflow Type and classify the exact marker set and Event History. A zero running count identifies no immediate live blocker, but it does not prove that pre-patch histories have left retention. Replay selected histories to test compatibility; sampled replay does not prove the retained population is empty. | ||
|
|
There was a problem hiding this comment.
| `TemporalChangeVersion IS NULL` is reliable only when no other patch marker could make the attribute non-null. For multiple or optional patch sites, inspect all retained executions of the Workflow Type and classify the exact marker set and Event History. A zero running count identifies no immediate live blocker, but it does not prove that pre-patch histories have left retention. Replay selected histories to test compatibility; sampled replay does not prove the retained population is empty. | |
| `TemporalChangeVersion IS NULL` is reliable only when no other same-id patch marker could make the attribute non-null. For multiple or optional patch sites, inspect all retained executions of the Workflow Type and classify the exact marker set and Event History. A zero running count identifies no immediate live blocker, but it does not prove that pre-patch histories have left retention. Replay selected histories to test compatibility; sampled replay does not prove the retained population is empty. |
| ### Retirement Lifecycle | ||
|
|
||
| **Step 1: Add GetVersion with both code paths** | ||
| Treat `GetVersion` cleanup as replay-safety work, not ordinary dead-code removal. Removing an old branch and deleting the first marker call are separate changes with different gates. |
There was a problem hiding this comment.
| Treat `GetVersion` cleanup as replay-safety work, not ordinary dead-code removal. Removing an old branch and deleting the first marker call are separate changes with different gates. | |
| `GetVersion` requires care to remain replay safe. Removing an old branch and deleting the first marker call are separate changes with different gates. |
| For new executions, `GetVersion` returns `1` and records a marker. For replay of pre-change workflows (no marker), it returns `DefaultVersion` (`-1`). | ||
| For new executions, `GetVersion` returns `1` and records a marker. For replay of pre-change workflows with no marker for `Step1`, it returns `workflow.DefaultVersion` (`-1`). **`DefaultVersion` is not version `0`**; do not search for `Step1-0` unless the code actually recorded version `0`. | ||
|
|
||
| #### Step 2: Inventory and prove the old version has left retention |
There was a problem hiding this comment.
This section got really big in Go for some reason, but not the others (the smaller versions seem preferable. There's a lot of redundancy with the non-language-specific file).
|
Ah sorry to waste your time on AI slop, but thanks for the feedback all the same. I'll take a second pass at this sometime this week. |
Summary
Upstreams hardened versioning changes from our internal skills.
workflow.GetVersionretirement procedure covering recorded versions versusDefaultVersion, retention gates, marker placement/removal, and permanentchangeIDretirementValidation
git diff --check🤖 RZ's Aimee