Drop pointers for domain name/id fields #3916
Conversation
| // send a signal on third failure to be buffered, forcing a non-transient decision when buffer is flushed | ||
| /*if failureCount == 3 { | ||
| err := s.engine.SignalWorkflowExecution(createContext(), &types.SignalWorkflowExecutionRequest{ | ||
| Domain: common.StringPtr(s.domainName), | ||
| Domain: s.domainName, |
There was a problem hiding this comment.
commented out?
not relevant for this diff, but is this something we should restore?
| @@ -470,7 +470,7 @@ func (t *taskAckManagerImpl) generateFailoverMarkerTask( | |||
| TaskType: types.ReplicationTaskType.Ptr(types.ReplicationTaskTypeFailoverMarker), | |||
| SourceTaskID: common.Int64Ptr(taskInfo.GetTaskID()), | |||
| FailoverMarkerAttributes: &types.FailoverMarkerAttributes{ | |||
| DomainID: common.StringPtr(taskInfo.GetDomainID()), | |||
There was a problem hiding this comment.
some of these original lines are a bit facepalm-ridden :|
e.g. this one takes a nullable *string, converts it to a non-null string defaulting to "", then gets a pointer back to *string. effectively just forgetting if the original was null or not.
well. this is why I like this PR, I suppose.
There was a problem hiding this comment.
(same as on wid/rid PR)
Well. The nullable pointer -> non-null pointer changes in the mappers are quite hard to trace and verify... so I'm not sure what can be done beyond "yolo, test it in prod". or spend an incredible amount of time trying to verify it all.
wid/rid/domain all seem pretty likely to always be present where required, so odds of this working are pretty good I think. works for me 👍
What changed?
Drop pointers for domain name/id fields within internal types. This is mostly mechanical change touching only those fields which are always required. This removes
common.StringPtrhelpers everywhere and modifies checks for empty domain instead of nil in handler validation. This is equivalent to previous logic, because incoming nil from request was masked everywhere withGetDomainhelper, which returned empty string anyway. Now this is done at the mapping layer.There were a few cases were domain id could be optional. In particular:
ParentExecutionInfo.DomainUUID/ParentExecutionInfo.DomainWorkflowExecutionInfo.ParentDomainIDWorkflowExecutionStartedEventAttributes.ParentWorkflowDomainActivityTaskScheduledEventAttributes.DomainIn those cases there were additional logic attached to
!= nilcase. I will check if it makes sense to use zero value for those and address that in a separate PR later if needed.Why?
This is one of PRs planned to drop pointers for always required fields in internal types. It will make them more idiomatic and easier to use. These pointer are legacy of Thrift types that were used previously through our codebase. This will also allow safer migration to grpc, as proto primitive types don't have nils by default.
How did you test it?
Passing build and existing tests.
Potential risks