Use common ParentExecutionInfo in proto IDLs #3997

vytautas-karpavicius · 2021-02-18T13:59:39Z

What changed?
Use common api.v1.ParentExecutionInfo to group fields related to parent execution.

Why?
So, the primary reason that have led to this proposal, is the fact that all parent related fields are optional. To properly represent such fields we would need to wrap them with wrapper types. However since they always come together, it makes sense to group them into separate type which then can then carry nullable semantics for all of them.

We already have such type which was used within internal API (shared.v1.ParentExecutionInfo). There are two additional places in publicly exposed API api.v1.WorkflowExecutionStartedEventAttributes and api.v1.WorkflowExecutionInfo that also have parent execution information. However not all of the fields are exposed there. One solution would to unify exposed fields and just single type to represent that. This is nice because it:

Unifies exposed parent execution information
Automatically provides not set semantics and groups fields that can be set all or none.
This does mean, that additional missing fields needs to be filled in (this PR attempts to do that). We can leave thrift IDLs as is, but they will be added for proto.

Some alternatives, that would work as well, but not as nice IMHO.

Simply use wrapper types to achieve optional semantics and leave fields ungrouped.
Use different grouping message(s) instead of reusing one from internal API.

How did you test it?

Potential risks

coveralls · 2021-02-18T15:15:29Z

Coverage decreased (-0.07%) to 64.568% when pulling 76de2cf on vytautas-karpavicius:proto-parent-execution-info into 80b7bfc on uber:master.

vytautas-karpavicius · 2021-02-19T14:42:02Z

There is also a similar situation for external_workflow_execution + external_initiated_event_id within history.RequestCancelWorkflowExecutionRequest. They are both either set or not. Depending on the decision made with this PR, we should be consistent and do the same thing there. So, for instance we could have:

message ExternalExecutionInfo {
  api.v1.WorkflowExecution workflow_execution = 1;
  int64 initiated_id = 2;
}

It also begs a question why external execution does not come with domain fields, as does parent execution.

yux0 · 2021-02-19T22:04:52Z

service/history/execution/state_builder.go

-			if attributes.ParentWorkflowDomain != nil {
+			if attributes.ParentWorkflowDomainID != nil {
+				parentDomainID = attributes.ParentWorkflowDomainID
+			} else if attributes.ParentWorkflowDomain != nil {


only domain or domain ID will be set in the attribute?

Both would be set. But for older history ParentWorkflowDomainID is not available, thus it would fallback to previous logic.

probably worth a code comment for that, as it's a bit unclear from the code alone. it's pretty clear it's a fallback, but not about when it may no longer be necessary (if ever).

Added a comment.

yux0 · 2021-02-19T22:05:27Z

service/history/historyEngine.go

@@ -1501,6 +1501,10 @@ func (e *historyEngineImpl) DescribeWorkflowExecution(
 			RunID:      executionInfo.ParentRunID,
 		}
 		result.WorkflowExecutionInfo.ParentDomainID = common.StringPtr(executionInfo.ParentDomainID)
+		result.WorkflowExecutionInfo.ParentInitiatedID = common.Int64Ptr(executionInfo.InitiatedID)
+		if entry, err := e.getActiveDomainEntry(executionInfo.ParentDomainID); err != nil {


Yes, great catch! Fixed it.

Groxx · 2021-02-25T05:17:57Z

You've committed the cadence-bench binary btw

Groxx

It also begs a question why external execution does not come with domain fields, as does parent execution.

I was wondering that while looking at that message, yeah... that said, I think all of these endpoints require you to pass in a domain? So in principle none of them need domain info AFAICT (though I don't see much of a downside to including them). Though I would be interested if there are counter-examples.

That said: tbh I don't really follow what the "external" workflow implies. If anything though, given the name, it seems like it'd be more appropriate for it to have domain info than for WorkflowExecutionInfo / etc.

Either way: consistency++, and these yeah, these fields all seem strongly inter-related, a separate message container makes a lot of sense IMO. I'm not really seeing a downside, so 👍 from me. Check comments of course, as you always do, and it may be worth getting another pair of eyes / waiting for @yux0? I'll leave that up to you tho.

Groxx · 2021-02-25T05:37:29Z

service/history/execution/state_builder.go

-			if attributes.ParentWorkflowDomain != nil {
+			if attributes.ParentWorkflowDomainID != nil {
+				parentDomainID = attributes.ParentWorkflowDomainID
+			} else if attributes.ParentWorkflowDomain != nil {


probably worth a code comment for that, as it's a bit unclear from the code alone. it's pretty clear it's a fallback, but not about when it may no longer be necessary (if ever).

Groxx · 2021-02-25T06:00:25Z

proto/public/uber/cadence/api/v1/history.proto

@@ -90,31 +90,29 @@ message HistoryEvent {

 message WorkflowExecutionStartedEventAttributes {


the fairly-large similarity between this and WorkflowExecutionInfo had me comparing things out of curiosity for a while... they may not all make sense to exist in WorkflowExecutionInfo (e.g. identity is per-event, not per-workflow), but I wonder if some of the missing fields would be useful to bring over.

e.g. when listing or describing a workflow:

input/result seem potentially useful (though possibly large data on list endpoints - defaulting to null makes sense, could be filled in when explicitly requested)

original / continue run_id seems like something that should exist

cron schedule and retry policy are almost definitely relevant to people looking at a Describe

*_start_to_close_timeouts are workflow-wide, possibly relevant. (... though now that I think about it, it's a little strange that those are not modify-able as a workflow runs. the use would be fairly rare, probably?)

Groxx · 2021-02-25T06:15:04Z

service/history/execution/history_builder.go

@@ -576,6 +576,7 @@ func (b *HistoryBuilder) newWorkflowExecutionStartedEvent(

 	parentInfo := startRequest.ParentExecutionInfo
 	if parentInfo != nil {
+		attributes.ParentWorkflowDomainID = parentInfo.DomainUUID


Not related to this PR except by proximity, but:
This kind of code:

attributes := &types.WorkflowExecutionStartedEventAttributes{} attributes.WorkflowType = request.WorkflowType attributes.TaskList = request.TaskList ...

Is rather hard to lint, e.g. that fields aren't forgotten. Or to be confident of it while coding.
It currently looks complete, but may be worth rewriting to a:

attributes := &types.WorkflowExecutionStartedEventAttributes{ WorkflowType: request.WorkflowType, TaskList: request.TaskList, Header: request.Header, Input: request.Input, ExecutionStartToCloseTimeoutSeconds: common.Int32Ptr(*request.ExecutionStartToCloseTimeoutSeconds),

Initializer (with an if for parentinfo stuff, since there's not much choice there).

As a concrete example, I can ctrl-space in that initializer and immediately see that 3 fields are missing - the parentinfo ones, handled below. I have yet to see tooling that helps even that much with the field-assignment pattern.

I've got a 90%-complete linter that checks for "complete" struct initializations, and it wouldn't be too hard to include "this func contains an assignment for every field" info in that check too. Maybe it'd be worth finishing and bringing in? Incomplete-initialization has been a fairly large source of sometimes-minor, often-long-lasting data losses in my experience.

Yes, I do prefer direct initialization as well. Updated it.

If we could lint on that, it would be amazing. Just wondering how it is handled when some fields are intentionally left out. Do we add annotation in the code?

vytautas-karpavicius · 2021-02-25T10:36:59Z

You've committed the cadence-bench binary btw

Thanks for noticing. Removed.

vytautas-karpavicius · 2021-02-25T10:41:15Z

It also begs a question why external execution does not come with domain fields, as does parent execution.

I was wondering that while looking at that message, yeah... that said, I think all of these endpoints require you to pass in a domain? So in principle none of them need domain info AFAICT (though I don't see much of a downside to including them). Though I would be interested if there are counter-examples.

That said: tbh I don't really follow what the "external" workflow implies. If anything though, given the name, it seems like it'd be more appropriate for it to have domain info than for WorkflowExecutionInfo / etc.

Either way: consistency++, and these yeah, these fields all seem strongly inter-related, a separate message container makes a lot of sense IMO. I'm not really seeing a downside, so 👍 from me. Check comments of course, as you always do, and it may be worth getting another pair of eyes / waiting for @yux0? I'll leave that up to you tho.

Thanks for all your insights.

Yep, will wait for second pair of eyes on this one. Kind of important changes happening here.

Use common ParentExecutionInfo in proto IDLs

dc51e3c

vytautas-karpavicius requested a review from a team February 18, 2021 13:59

yux0 reviewed Feb 19, 2021

View reviewed changes

vytautas-karpavicius added 2 commits February 22, 2021 13:58

Fix err == nil

1ff3fe5

Wrap ExternalExecutionInfo

cafce60

Groxx approved these changes Feb 25, 2021

View reviewed changes

vytautas-karpavicius added 4 commits February 25, 2021 12:20

Removed unintention commit - cadence-bench

cf6d0a0

Additional comment

b25b4bd

Use initializer for WorkflowExecutionStartedEventAttributes

60278a4

Merge branch 'master' into proto-parent-execution-info

d6787f8

Fix after merge

0ecabb5

yux0 approved these changes Feb 25, 2021

View reviewed changes

vytautas-karpavicius added 2 commits February 26, 2021 09:20

Merge branch 'master' into proto-parent-execution-info

6fd0a6d

Fix after merge

76de2cf

vytautas-karpavicius merged commit c5d67e3 into cadence-workflow:master Feb 26, 2021

yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021

Use common ParentExecutionInfo in proto IDLs (cadence-workflow#3997)

130c37a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use common ParentExecutionInfo in proto IDLs #3997

Use common ParentExecutionInfo in proto IDLs #3997

vytautas-karpavicius commented Feb 18, 2021

coveralls commented Feb 18, 2021 •

edited

Loading

vytautas-karpavicius commented Feb 19, 2021

yux0 Feb 19, 2021

vytautas-karpavicius Feb 22, 2021

Groxx Feb 25, 2021

vytautas-karpavicius Feb 25, 2021

yux0 Feb 19, 2021

vytautas-karpavicius Feb 22, 2021

Groxx commented Feb 25, 2021

Groxx left a comment

Groxx Feb 25, 2021

Groxx Feb 25, 2021

Groxx Feb 25, 2021

vytautas-karpavicius Feb 25, 2021

vytautas-karpavicius commented Feb 25, 2021

vytautas-karpavicius commented Feb 25, 2021

		@@ -90,31 +90,29 @@ message HistoryEvent {

		message WorkflowExecutionStartedEventAttributes {

Use common ParentExecutionInfo in proto IDLs #3997

Use common ParentExecutionInfo in proto IDLs #3997

Conversation

vytautas-karpavicius commented Feb 18, 2021

coveralls commented Feb 18, 2021 • edited Loading

vytautas-karpavicius commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx commented Feb 25, 2021

Groxx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vytautas-karpavicius commented Feb 25, 2021

vytautas-karpavicius commented Feb 25, 2021

coveralls commented Feb 18, 2021 •

edited

Loading