Wait for user data propagation in deployment workflow #6978
Conversation
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case err := <-complete: | ||
| return err |
There was a problem hiding this comment.
Should we log the error? Also, I'm going to make task for adding metrics and alerts we need them for across the board for effective monitoring of user data processing and deployment WFs.
There was a problem hiding this comment.
err can't be anything other than nil here. I just had it return it for future extensibility. this never returns an error that isn't a context error. it's a slightly different semantics from the other long poll stuff that can return "empty"/"nothing yet" as a success response. I can do that if desired but it seems like extra complication?
if the matchingClient call above fails, the client itself will log matching client encountered error, so we don't have to do it. well, it will not log DeadlineExceeded/Canceled, which I think is what we want.
it will log FailedPrecondition, which I want to ignore here. I think we can live with that.
| @@ -302,11 +311,19 @@ func (d *DeploymentWorkflowRunner) handleSyncState(ctx workflow.Context, args *d | |||
| } | |||
| } | |||
| activityCtx := workflow.WithActivityOptions(ctx, defaultActivityOptions) | |||
| err = workflow.ExecuteActivity(activityCtx, d.a.SyncUserData, syncReq).Get(ctx, nil) | |||
| var syncRes deploymentspb.SyncUserDataResponse | |||
| err = workflow.ExecuteActivity(activityCtx, d.a.SyncUserData, syncReq).Get(ctx, &syncRes) | |||
There was a problem hiding this comment.
Do you think we'd need to limit the number of TQs we pass to each Activity call here? It seems 1000 TQs at one time can become very fragile.
There was a problem hiding this comment.
yeah, I can add batching in the next PR
| if err != nil { | ||
| // TODO: if this fails, should we roll back anything? | ||
| return nil, err | ||
| } | ||
| // wait for propagation | ||
| err = workflow.ExecuteActivity(activityCtx, d.a.CheckUserDataPropagation, &deploymentspb.CheckUserDataPropagationRequest{ |
There was a problem hiding this comment.
Same for this call. In here the problem would be more sever because there is another level of fanout to all partitions.
There was a problem hiding this comment.
good point, batching sounds good
| if err != nil { | ||
| // TODO: if this fails, should we roll back anything? | ||
| return nil, err | ||
| } | ||
| // wait for propagation | ||
| err = workflow.ExecuteActivity(activityCtx, d.a.CheckUserDataPropagation, &deploymentspb.CheckUserDataPropagationRequest{ |
There was a problem hiding this comment.
Need to path for this change, right?
There was a problem hiding this comment.
ugh, I was assuming we wouldn't start using this before these changes or could just reset everything...
how much do we have to be concerned with an in-progress deployment and an activity task from the new worker getting sent to an old worker that doesn't know about it?
actually, we can do both at once: only do this if syncRes.TaskQueueMaxVersions has data. replays and older activity handlers will both have no data there. that seems to work.
What changed?
The deployment workflow waits for user data to propagate to all task queue partitions before updating its state.
Why?
We should ensure that the desired dispatch semantics will be in effect on all task queue partitions.
How did you test it?
existing tests, new unit test