Clone request if search attributes are mapped #3299
Conversation
alexshtin
force-pushed
the
feature/sa-clone-request
branch
from
c02da29 to
b95ff74
Compare
service/frontend/workflow_handler.go
Outdated
| } | ||
|
|
||
| func (wh *WorkflowHandler) mapCreateScheduleRequestStartWorkflowSearchAttributes(request *workflowservice.CreateScheduleRequest, namespaceName namespace.Name) (*workflowservice.CreateScheduleRequest, error) { | ||
| if startWorkflow := request.GetSchedule().GetAction().GetStartWorkflow(); startWorkflow != nil { |
There was a problem hiding this comment.
flip this conditional so you can unindent most of this?
also this has to be in terms of schedulepb.Schedule, not CreateScheduleRequest, and one copy/replace has to move to the caller. you'll see when you merge...
| handler.stopProcessing = true | ||
| return err | ||
| } | ||
| if mappedSearchAttributes != nil { | ||
| newAttr := *attr |
There was a problem hiding this comment.
:-) This is another attr. This is command attributes, not search.
What could the interceptor do except clone the proto itself? If I were doing this from scratch, I think it might be better to go the other way and push the aliasing way down the stack, into visibility. So we're not touching the request at all, but we do it when we build the ES doc or whatever SQL thing. Or maybe slightly higher, after search attributes have already been split out of the request and attached to other metadata. We could still "dry-run" the mapping in the handler to validate. Then we shouldn't get errors later, but if so just ignore them. |
alexshtin
force-pushed
the
feature/sa-clone-request
branch
from
a2e2a9e to
35a3ef1
Compare
common/searchattribute/mapper.go
Outdated
| func ApplyAliases(mapper Mapper, searchAttributes *commonpb.SearchAttributes, namespace string) error { | ||
| // ApplyAliases returns SearchAttributes struct where each search attribute name is replaced with alias. | ||
| // If no replacement where made, it returns nil which means that original SearchAttributes struct should be used. | ||
| func ApplyAliases(mapper Mapper, searchAttributes *commonpb.SearchAttributes, namespace string) (*commonpb.SearchAttributes, error) { |
There was a problem hiding this comment.
As long as this is still open...
Would it be possible to rename these to make it obvious that ApplyAliases and SubstituteAliases are inverses? E.g. ApplyAliases/UnapplyAliases or AliasFields/UnaliasFields?
alexshtin
force-pushed
the
feature/sa-clone-request
branch
from
35a3ef1 to
27756a5
Compare
|
I am against reflection, especially on such hot path. I could consider moving all these ugly funcs to dedicated package/struct and forget about them. |
alexshtin
force-pushed
the
feature/sa-clone-request
branch
from
27756a5 to
48ab1be
Compare
alexshtin
force-pushed
the
feature/sa-clone-request
branch
from
48ab1be to
70d148c
Compare
What changed?
Clone request if search attributes are mapped.
Why?
To insure idempotence of handler functions.
How did you test it?
Exixting tests.
Potential risks
No risks.
Is hotfix candidate?
No.