Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FirstExecutionRunID to mutable state #5031

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

Shaddoll
Copy link
Contributor

@Shaddoll Shaddoll commented Nov 19, 2022

What changed?
Added FirstExecutionRunID to mutable state which gets persisted in DB. Now we don't rely on workflow execution started event for this information.

This partially picks temporalio/temporal#365

Why?
See temporalio/temporal#365

How did you test it?
unit test, integration test and local test on laptop

Potential risks
There might be backward compatibility issue, and we must make sure the old mutable state can still be read from database.

Release notes

Documentation Changes

@Shaddoll Shaddoll force-pushed the fix-parent-close-policy branch 9 times, most recently from 1773fa9 to 05dcc48 Compare November 21, 2022 22:20
@Shaddoll Shaddoll changed the title [WIP]Add FirstExecutionRunID to mutable state Add FirstExecutionRunID to mutable state Nov 21, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 0184a235-4b7a-4d76-8b8c-b3b9c34ab87d

  • 25 of 69 (36.23%) changed or added relevant lines in 12 files are covered.
  • 176 unchanged lines in 18 files lost coverage.
  • Overall coverage decreased (-0.05%) to 57.267%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/history/execution/mutable_state_util.go 0 1 0.0%
common/persistence/nosql/nosqlplugin/cassandra/workflowParsingUtils.go 6 8 75.0%
service/history/execution/mutable_state_builder.go 9 11 81.82%
common/persistence/persistence-tests/executionManagerTestForEventsV2.go 0 3 0.0%
common/persistence/persistence-tests/persistenceTestBase.go 0 3 0.0%
common/persistence/nosql/nosqlExecutionStoreUtil.go 2 6 33.33%
common/persistence/serialization/getters.go 0 5 0.0%
common/persistence/persistence-tests/executionManagerTest.go 0 24 0.0%
Files with Coverage Reduction New Missed Lines %
common/task/weightedRoundRobinTaskScheduler.go 1 89.64%
common/persistence/persistenceMetricClients.go 2 55.45%
service/history/shard/context.go 2 66.52%
common/membership/hashring.go 2 83.54%
service/history/task/transfer_standby_task_executor.go 2 87.66%
service/history/execution/mutable_state_util.go 2 36.04%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go 3 60.0%
common/persistence/statsComputer.go 3 94.64%
service/history/task/fetcher.go 3 91.75%
common/cache/lru.go 5 90.73%
Totals Coverage Status
Change from base Build 0184a0a5-6f34-4c09-909a-9a21e555cf31: -0.05%
Covered Lines: 85117
Relevant Lines: 148631

💛 - Coveralls

@@ -152,6 +153,7 @@ func FromInternalWorkflowExecutionInfo(executionInfo *persistence.InternalWorkfl
CompletionEventEncoding: string(common.EncodingTypeEmpty),
VersionHistoriesEncoding: string(common.EncodingTypeEmpty),
InitiatedID: common.EmptyEventID,
FirstExecutionRunID: MustParseUUID(executionInfo.FirstExecutionRunID),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possibility of this being an invalid UUID? I'm always a little scared of using MustParse for places like this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking below at the builder - from a naïve perspective this looks maybe it could be an invalid uuid, or at least a nil one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MustParseUUID works with empty string

@Shaddoll Shaddoll merged commit d249434 into uber:master Nov 23, 2022
@Shaddoll Shaddoll deleted the fix-parent-close-policy branch November 23, 2022 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants