fix(active-active): Fix auto-forwarding for active-active domains #7356
Conversation
Shaddoll
requested review from
3vilhamster,
Groxx,
davidporter-id-au,
demirkayaender,
dkrotx,
jakobht,
neil-xie,
sankari165,
shijiesheng and
taylanisikdemir
as code owners
| if actClSelPolicyForNewWF != nil { | ||
| policy.logger.Debug("Active cluster selection policy for new workflow", tag.WorkflowDomainName(domainEntry.GetInfo().Name), tag.OperationName(apiName), tag.Dynamic("policy", actClSelPolicyForNewWF)) | ||
| activeClusterInfo, err := policy.activeClusterManager.GetActiveClusterInfoByClusterAttribute(ctx, domainEntry.GetInfo().ID, actClSelPolicyForNewWF.GetClusterAttribute()) | ||
| if apiName == "SignalWithStartWorkflowExecution" { |
There was a problem hiding this comment.
Switching on API name/string raises some red flags for me. I know they should be static and shouldn't change - but a change to the way we generate our APIs would cause a breaking change here.
Is there a way we could implement the logic without knowing which API is calling it? Perhaps just from the information we have in the non-api name parameters? Previously it was only used for logging tags.
e.g if the actClSelPolicyForNewWF is not nil we could always check for an an existing policy (as that covers the signalwithstart case as well).
There was a problem hiding this comment.
What about a switch block which panics on fallthrough / default to ensure that we catch these failures noisily on refactor?
There was a problem hiding this comment.
I think that would make it easier to track, but I'm not concerned about the refactor as much as I am maintaining this in a couple years when we try to change our proto gen, or someone copying this pattern elsewhere.
There was a problem hiding this comment.
It has to know the API name, because actClSelPolicyForNewWF can be empty for StartWorkflow and SignalWithStartWorkflow API. And the forwarding rules are API-based. The function needs to need the APIs directly or indirectly.
3 participants
What changed?
Why?
Bug fix
How did you test it?
unit tests && simulation tests
Potential risks
Release notes
Documentation Changes