Ensure WD Version properly revives if recreated after deletion#9382
Ensure WD Version properly revives if recreated after deletion#9382
Conversation
- Use InDelta instead of Equal for float comparison - Use s.Require().NoError for error assertions in callbacks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ShahabT
changed the title
| } | ||
| }() | ||
|
|
||
| if c.deploymentVersionRegistered { |
There was a problem hiding this comment.
@Shivs11 note that this was a separate obstacle preventing the TQ being registered after revived if the partition is not reloaded since last registration.
There was a problem hiding this comment.
I wonder: I know the userData calls below that you have mentioned are all in-mem calls, but if a partition has a ton of pollers, would that be a lot on our matching engine pod?
There was a problem hiding this comment.
the user data is in memory and access to it should not be expensive for each poll.
There was a problem hiding this comment.
actually, I don't think every poll needs to wait for the lock. I added a user date check before the lock.
| asyncPropagationsInProgress int | ||
| // When true, all the ongoing propagations should cancel themselves | ||
| // Deprecated. With version data revision number, we don't need to cancel propagations anymore. | ||
| // Used when delete happens while there are ungoing propagations. |
| // And there is a force CaN or a propagated state change or history got too large | ||
| (d.forceCAN || (d.stateChanged && d.asyncPropagationsInProgress == 0) || workflow.GetInfo(ctx).GetContinueAsNewSuggested())) |
There was a problem hiding this comment.
im confused here - why did we never have this before? workflow.GetInfo(ctx).GetContinueAsNewSuggested()
moreover, aren't we always bound to CAN before we actually hit this limit? or did you get this idea from the recent investigation we noticed where we had 10,000 or so signals but we sadly were not can'ing?
There was a problem hiding this comment.
one other point:
when CaN triggers via this path, d.stateChanged might be false and asyncPropagationsInProgress might be > 0. The condition allows CaN even with in-flight propagations when history is too large. Is that intentional? I wonder if this could leave our poor TQ in a really poor state then
There was a problem hiding this comment.
The condition allows CaN even with in-flight propagations
I don't think this is the case, the previous clause is still in effect:
(!d.signalHandler.signalSelector.HasPending() && d.signalHandler.processingSignals == 0 && workflow.AllHandlersFinished(ctx) &&
| } | ||
| } else if v := req.GetForgetVersion(); v != nil { | ||
| if idx := worker_versioning.FindDeploymentVersion(deploymentData, v); idx >= 0 { | ||
| // Go through the new and old deployment data format for this deployment and remove the version if present. |
There was a problem hiding this comment.
in the helper function, right now, we return early if workerDeploymentData == nil. In that manner, we won't go through the old format no? (say the version was present in the deploymentsData format only)
afaik, you are trying to merge these two deletion paths (which is great), but this could be something you wanna think off
There was a problem hiding this comment.
good point, I'm gonna fix that case.
| } | ||
| }() | ||
|
|
||
| if c.deploymentVersionRegistered { |
There was a problem hiding this comment.
I wonder: I know the userData calls below that you have mentioned are all in-mem calls, but if a partition has a ton of pollers, would that be a lot on our matching engine pod?
…ersion # Conflicts: # service/worker/workerdeployment/version_workflow_test.go # service/worker/workerdeployment/workflow_test.go
…ralio#9382) ## What changed? - Ensure the TQs receive and apply the right version data after revive. - Made delete propagation to always happen serial to other propagations. It ensures all other propagations are cancelled before starting delete propagation. - Deprecate the `deleted` flag in version data and the GC logic around it. Now we use the good old forgetVersion path which immediately removes the version data from TQ. - Ensure version state is reset after revive, in case the recreation happened before workflow close. - Also, now workflows CaN based on SDK suggestion if no pending Signal or Update is present. ## Why? The version could stuck at deleted state from TQ POV if revived before the (now deprecated) GC logic cleans it up. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s) ## Potential risks None --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ralio#9382) ## What changed? - Ensure the TQs receive and apply the right version data after revive. - Made delete propagation to always happen serial to other propagations. It ensures all other propagations are cancelled before starting delete propagation. - Deprecate the `deleted` flag in version data and the GC logic around it. Now we use the good old forgetVersion path which immediately removes the version data from TQ. - Ensure version state is reset after revive, in case the recreation happened before workflow close. - Also, now workflows CaN based on SDK suggestion if no pending Signal or Update is present. ## Why? The version could stuck at deleted state from TQ POV if revived before the (now deprecated) GC logic cleans it up. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s) ## Potential risks None --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
What changed?
deletedflag in version data and the GC logic around it. Now we use the good old forgetVersion path which immediately removes the version data from TQ.Why?
The version could stuck at deleted state from TQ POV if revived before the (now deprecated) GC logic cleans it up.
How did you test it?
Potential risks
None