Support unpinned workflows

[ShahabT]

What changed?

Add Matching and History changes to properly route unpinned workflow tasks.

Why?

How did you test it?

Tests will come in separate PR.

Potential risks

Documentation

Is hotfix candidate?

[alexshtin]

I would use GetXXX() format everywhere for consistency. Also all these log tag thing is aiming to 0 allocations and 0 overhead if tag is not used but I guess there is no good way to make it work here.

[alexshtin]

I would make deployment a string here and call DeploymentToString before passing deployment to this func. Let's make tags trivial simple.

[dnr]

In this case there already is a deployment != nil check so I'd just use direct field access.

Even better: I think you can delay the allocation until the tag is used by doing something like:

type deploymentStringer *deploymentpb.Deployment

// implement fmt.Stringer
func (d deploymentStringer) String() string {
  if pb := (*deploymentpb.Deployment)(d); pb != nil {
    return pb.SeriesName + ":" + pb.BuildId
  }
  return "unversioned"
}

func Deployment(d *deploymentpb.Deployment) ZapTag {
  return NewAnyTag("deployment", deploymentStringer(d))
}

[ShahabT]

I tried the stringer thing but it gave me "Invalid receiver type 'deploymentStringer' ('*deploymentpb.Deployment' is a pointer type)" error. So I got rid of it, still improved the implementation.

[dnr]

oh right.. I think you can work around that restriction by making a one-field struct with the pointer and using that as a value

[ShahabT]

I figured but I'm not sure how much value the delayed allocation adds in this case, is it worth all the extra code?

[alexshtin]

Can you specify TaskQueueData.last_became_current_time?

[alexshtin]

This nested Deployment is a bad sign. DeploymentDataItem?

[alexshtin]

Same way as we refer "workflow task" as WFT everywhere in comments, I would suggest to refer "activity task" as AT, not just "task".

[alexshtin]

Suggested change

	dispatchDeployment := worker_versioning.DeploymentFromCapabilities(req.PollRequest.WorkerVersionCapabilities)
	pollerDeployment := worker_versioning.DeploymentFromCapabilities(req.PollRequest.WorkerVersionCapabilities)

[alexshtin]

Suggested change

	directiveDeployment := req.GetDirectiveDeployment()
	scheduledDeployment := req.GetScheduledDeployment()

-- deployment which history wanted when it scheduled a WFT.

[alexshtin]

Suggested change

	return nil, serviceerrors.NewObsoleteMatchingTask("workflow is not eligible for transition")
	return nil, serviceerrors.NewObsoleteMatchingTask(workflow.ErrPinnedWorkflowCannotTransition.Error())

[dnr]

partial review, will finish the rest in an hour or two

[dnr]

In this case there already is a deployment != nil check so I'd just use direct field access.

Even better: I think you can delay the allocation until the tag is used by doing something like:

type deploymentStringer *deploymentpb.Deployment

// implement fmt.Stringer
func (d deploymentStringer) String() string {
  if pb := (*deploymentpb.Deployment)(d); pb != nil {
    return pb.SeriesName + ":" + pb.BuildId
  }
  return "unversioned"
}

func Deployment(d *deploymentpb.Deployment) ZapTag {
  return NewAnyTag("deployment", deploymentStringer(d))
}

[dnr]

uh oh, this was a mess to merge before since the logic got extracted. see what I did in the merge commit and if you like that or want to do it differently

[dnr]

rename as discussed

[dnr]

if this doesn't work for pinned, which is the call to move a pinned / change the pinned state?

[ShahabT]

Not sure I understand the question. Pinned state normally is set in afterAddWorkflowTaskCompletedEvent, also could come from manual override which is applied in ApplyWorkflowExecutionOptionsUpdatedEvent.

[dnr]

shouldn't this be

Suggested change

	tag.WorkerBuildId(db.queue.Version().MetricsTagValue()), tag.WorkflowNamespace(db.queue.Partition().NamespaceId()))
	tag.WorkerBuildId(db.queue.Version().MetricsTagValue()), tag.WorkflowNamespaceId(db.queue.Partition().NamespaceId()))

? mixing up name/id

[dnr]

might as well change this to keep the pointer instead of the index

[dnr]

the log tag boilerplate in this function is getting a little out of hand. let's add some helpers later?

[dnr]

wasn't this in the previous pr? do you need to merge so we only see the diff from this one?

[ShahabT]

yes it was, and the previous PR is merged, it might be that Github has not updated the diff?! I'm going to merge some changes now and hopefully the diff gets refreshed.

[alexshtin]

You need to move this check above and don't start transition if WFT is speculative. It is because we don't want any extra changes in MS which are not persisted, but then decisions (like dropping AT) are made based on this, and then MS is lost.
And it is also ok not to do anything here because when new deployment is pinned new normal WFT is created and it will start transition.

[alexshtin]

This ^^^ comment probably worth to be moved in code comment (with some modifications).

[ShahabT]

Instead of moving the check up, I moved the StartDeploymentTransition call down. I hope it counts :D

[alexshtin]

Suggested change

	taskDeployment := attrs.GetDeployment()
	taskBehavior := attrs.GetVersioningBehavior()
	wftDeployment := attrs.GetDeployment()
	wftBehavior := attrs.GetVersioningBehavior()

same reason: I would avoid word "task" everywhere where possible.

[alexshtin]

It is very unclear that assignment on the line 1094 actually clears this. I would remove ms.GetDeploymentTransition() and operate versioningInfo here and everywhere else.

[alexshtin]

It can't be nil here. At least I don't see how.

[ShahabT]

It can: the wf was unversioned, hence its versioningInfo was nil. A speculative task redirected to a new deployment without touching versioningInfo at start time, and now it is completing with a non-nil wftDeployment.

[alexshtin]

Line 1089 will give NPE then.

[alexshtin]

It really feels like we need a message/struct which combines Deployment and Behavior.

[ShahabT]

I looked at the usages. Whenever Behavior is used, Deployment is also used, but not vice versa (in many places we only care about deployment). I think we can keep it as is.

[alexshtin]

Equal supports nil on both sides:

Suggested change

	if versioningInfo.DeploymentTransition != nil {
	// It's possible that the completed WFT is not yet from the current transition because when
	// the transition started, the current wft was already started. In this case, we allow the
	// started wft to run and when completed, we create another wft immediately.
	if versioningInfo.DeploymentTransition.GetDeployment().Equal(wftDeployment) {
	versioningInfo.DeploymentTransition = nil
	completedTransition = true
	}
	}
	// It's possible that the completed WFT is not yet from the current transition because when
	// the transition started, the current wft was already started. In this case, we allow the
	// started wft to run and when completed, we create another wft immediately.
	if versioningInfo.GetDeploymentTransition().GetDeployment().Equal(wftDeployment) {
	versioningInfo.DeploymentTransition = nil
	completedTransition = true
	}

[ShahabT]

We don't want to set completedTransition to true if there was no transition and wftDeployment is nil.

[alexshtin]

I think you can add early exit here. If versioningInfo is nil, all the rest doesn't need to be checked at all.

[alexshtin]

ok, I read a comment about speculative WFT :-). Now I believe that you just need to move that block with versionInfo initialization here.

[ShahabT]

can you make a suggested change? I'm not totally following.

[alexshtin]

Please add a comment saying that versionInfo can be nil.

[alexshtin]

Suggested change

	if versioningInfo.DeploymentTransition != nil {
	if versioningInfo.GetDeploymentTransition() != nil {