WorkflowExecutionInfo Field Conversions #715

shawnhathaway · 2020-09-09T01:47:23Z

What changed?
Incremental PR of changes on way to have MutableState be a proto everywhere. This PR:

Removes persistence.InternalWorkflowExecutionInfo - This can return later when we are happy with object structure.
Aligns WEI field types with expected proto generated types
Add time nil resiliency where accessed unsafely
Embed persistenceblobs.ExecutionStats in persistenceblobs.WorkflowExecutionInfo and deprecate HistorySize -- HistorySize needs to be dual written both as a field and in the new stats sub object until we validate complete migration and can remove the deprecated field. (v1.1+++)

Bug Fixes:

createMutableState/copyWorkflowExecutionInfo test util now copies WorkflowTaskScheduledTimestamp
historyEngine_test#TestRecordWorkflowTaskStarted***Sticky*** tests correctly compare response.ScheduledTime to the scheduled event written
timestamp.UnixOrZero now functions as UnixNano (to be renamed in seperate PR)
- Introduced timestamp.UnixOrEmpty here to replace this as zero is ambiguous

How did you test it?
Local integration, unit tests. Buildkite validation

Potential risks
Time is now nillable and we could have nil pointer exceptions hidden about. This is the first incremental PR and more work will be done on this before in upcoming PRs. The pipelines should give us extra coverage here.

- HistoryEvent EventTime is now *time.Time

shawnhathaway · 2020-09-09T01:50:44Z

common/primitives/timestamp/time.go

+	return TimePtr(UnixOrEmptyTime(nanos))
+}
+
+func UnixOrZeroTime(nanos int64) time.Time {


UnixOrZeroTime to be renamed to UnixNano in later PR - UnixOrEmptyTime will now return empty time object.

shawnhathaway · 2020-09-09T01:50:56Z

proto/internal/temporal/server/api/replication/v1/message.proto

 import "temporal/api/history/v1/message.proto";
 import "temporal/api/failure/v1/message.proto";

+message ReplicationInfo {


This was mistakenly removed in a prior commit. @alexshtin Appears we don't validate the build for proto?

We don't. You supposed to regenerate go files every time manually. I agree, we should add this step and check if there are any changes in api dir after make proto.

shawnhathaway · 2020-09-09T01:52:35Z

service/history/historyEngine_test.go

 		WorkflowTaskAttempt:                    sourceInfo.WorkflowTaskAttempt,
 		WorkflowTaskStartedTimestamp:           sourceInfo.WorkflowTaskStartedTimestamp,
 		WorkflowTaskOriginalScheduledTimestamp: sourceInfo.WorkflowTaskOriginalScheduledTimestamp,
+		WorkflowTaskScheduledTimestamp:         sourceInfo.WorkflowTaskScheduledTimestamp,


Long-standing bug that this was missing

shawnhathaway · 2020-09-09T01:53:36Z

service/history/historyEngine2_test.go

 	s.NotNil(response)
+	s.True(response.StartedTime.After(*expectedResponse.ScheduledTime))
 	expectedResponse.StartedTime = response.StartedTime
-	expectedResponse.ScheduledTime = &time.Time{}


ScheduledTime should match event time. This was a bug hidden by missing field in copyWorfklowExecutionInfo

shawnhathaway · 2020-09-09T01:54:37Z

service/history/mutableStateWorkflowTaskManager.go

-		ScheduledTimestamp:  m.msb.timeSource.Now().UnixNano(),
-		StartedTimestamp:    0,
+		ScheduledTimestamp:  timestamp.TimePtr(m.msb.timeSource.Now()),
+		StartedTimestamp:    timestamp.UnixOrZeroTimePtr(0),


Outstanding Todo: Initialize 0 values with nil instead of 0 -- Note this applies to Time only. Durations (ie Timeouts) of 0 vs nil has actual distinction.

shawnhathaway · 2020-09-10T02:10:36Z

common/persistence/persistenceInterface.go


+	// Back compat for GetHistorySize
+	if info.GetHistorySize() >= 0 && info.GetExecutionStats() == nil {
+		executionInfo.ExecutionStats = &persistenceblobs.ExecutionStats{HistorySize: 0}


Ignore bug here - fixed in a separate branch/commit with a test to ensure this works correctly migration. Will separate and apply to this PR shortly.

alexshtin · 2020-09-10T16:45:26Z

proto/internal/temporal/server/api/replication/v1/message.proto

 import "temporal/api/history/v1/message.proto";
 import "temporal/api/failure/v1/message.proto";

+message ReplicationInfo {


We don't. You supposed to regenerate go files every time manually. I agree, we should add this step and check if there are any changes in api dir after make proto.

alexshtin · 2020-09-10T19:33:52Z

common/persistence/dataInterfaces.go

+		WorkflowTaskStartedTimestamp           *time.Time
+		WorkflowTaskScheduledTimestamp         *time.Time
+		WorkflowTaskOriginalScheduledTimestamp *time.Time


I think these need to be renamed to *Time not *Timestamp.

alexshtin · 2020-09-10T19:35:35Z

common/persistence/persistenceInterface.go

 		WorkflowTaskScheduleId:            executionInfo.WorkflowTaskScheduleID,
 		WorkflowTaskStartedId:             executionInfo.WorkflowTaskStartedID,
 		WorkflowTaskRequestId:             executionInfo.WorkflowTaskRequestID,
-		WorkflowTaskTimeout:               timestamp.DurationFromSeconds(executionInfo.WorkflowTaskTimeout),


So ideally, when we finish all time conversion we should not use timestamp.DurationFromSeconds and timestamp.TimestampFromTime at all. (Maybe only when we read from configs, but even there we can use duration and time).

Definitely shouldn't need in serialization layers and agree with overall direction. However, some places we may still need it in code such as Persistence interfaces which take int64 timeouts in some areas to provide to cassandra as TTL (seconds only), whereas SQL layers may use timestamps.

alexshtin · 2020-09-10T19:54:34Z

common/persistence/persistence-tests/executionManagerTestForEventsV2.go

 				WorkflowTypeName:           "wType",
-				WorkflowRunTimeout:         20,
-				DefaultWorkflowTaskTimeout: 13,
+				WorkflowRunTimeout:         timestamp.DurationFromSeconds(20),


You lose beauty of go duration here. Wouldn't it be better: timestamp.DurationPtr(20 * time.Second)?

Imho, I prefer having DurationFromUnit() to avoid mistakes in math and make refactoring a bit simpler. Can always drop down to `DurationPtr() with more complex times. Lets chat more.

alexshtin · 2020-09-10T20:17:45Z

service/history/timerQueueActiveTaskExecutor_test.go

 	executionInfo := mutableState.executionInfo
 	executionInfo.HasRetryPolicy = true
-	executionInfo.WorkflowExpirationTime = s.now.Add(1000 * time.Second)
+	executionInfo.WorkflowExpirationTime = timestamp.TimeNowPtrUtcAddSeconds(1000)


I don't like this helper func :-). Yeah, definitely don't like.

alexshtin · 2020-09-10T20:28:02Z

service/history/transferQueueTaskExecutorBase.go

 		StartTimestamp:     startTimeUnixNano,
 		ExecutionTimestamp: executionTimeUnixNano,
-		RunTimeout:         int64(runTimeout),
+		RunTimeout:         int64(timestamp.DurationValue(runTimeout).Round(time.Second).Seconds()),


I expect all these rounds to be temporary, right?

Some of these will stay due to Cassandra TTL being in seconds.

…GetWorkflowExecution

shawnhathaway added 18 commits September 8, 2020 18:31

LastUpdatedTimestamp

2e200cc

Move ExecutionStats to be embedded in WEI

b34bf45

Remove Internal WEI

e2ac138

Use new helpers

1afc3a4

Init execution stats when history size is 0

5f9d449

WEI MaximalInterval -> *time.Duration

fe4e45f

WEI InitialInterval -> *time.Duration

4494383

WEI StickySchedToStart -> *time.Duration

3b3fe06

WEI WorkflowTaskTimeout & Default -> *time.Duration

46028c4

WEI WorkflowExecutionTimeout -> *time.Duration

f5ff7ce

WEI WorkflowRunTimeout -> *time.Duration

f17c3dd

WEI WorkflowStartedTimestamp -> *time.Time

11592a0

- HistoryEvent EventTime is now *time.Time

Test Fixes

8502c61

WEI WorkflowExpirationTime -> *time.Time

2878992

WEI StartTime/LastUpdatedTime -> *time.Time

2b00214

WEI OriginalScheduledTimestamp/WorkflowScheduledTimestamp -> *time.Time

8c9c877

Merge remote-tracking branch 'noir/master' into mutable

1a04be8

go fmt ./...

7d01598

shawnhathaway requested a review from alexshtin September 9, 2020 01:49

shawnhathaway commented Sep 9, 2020

View reviewed changes

shawnhathaway marked this pull request as ready for review September 9, 2020 02:56

Merge branch 'master' into mutable

3b8be21

shawnhathaway commented Sep 10, 2020

View reviewed changes

alexshtin approved these changes Sep 10, 2020

View reviewed changes

shawnhathaway and others added 2 commits September 10, 2020 13:40

Merge branch 'master' into mutable

36c1142

Merge branch 'master' into mutable

09b554e

shawnhathaway added 4 commits September 11, 2020 08:40

Copy Execution Stats, verify ExecutionInfo duplicates HistorySize in …

fab665f

…GetWorkflowExecution

Rename WEI ProtoConverters, move to protoConverter.go

8946b77

Add proto conversion testing for WEI

537cbe3

Merge branch 'master' into mutable

ace015d

shawnhathaway merged commit faf2fe7 into temporalio:master Sep 11, 2020

WorkflowExecutionInfo Field Conversions #715

WorkflowExecutionInfo Field Conversions #715

Uh oh!

Conversation

shawnhathaway commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnhathaway Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawnhathaway Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shawnhathaway commented Sep 9, 2020 •

edited

Loading

shawnhathaway Sep 9, 2020 •

edited

Loading

shawnhathaway Sep 9, 2020 •

edited

Loading