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.

Sign up for GitHub

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

Jump to bottom

Refactor peristence layer to support inserting history tasks of new categories #6671

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Refactor peristence layer to support inserting history tasks of new categories #6671

merged 1 commit into from
Feb 20, 2025

Conversation

Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented Feb 14, 2025 •
edited
Loading

Detailed Description
Introduce data and data_encoding columns to the executions table of Cassandra. These field are used to replace other columns in executions table. We'll start by replacing the history task columns (transfer, timer and replication).

Impact Analysis

  • Backward Compatibility: Yes. It only changes the schema, no data is written to the new columns in this PR.
  • Forward Compatibility: Yes. It only changes the schema, no data is written to the new columns in this PR.

Testing Plan

  • Unit Tests: Yes.
  • Persistence Tests: No. The code implementation is not fully done in this PR.
  • Integration Tests: No.
  • Compatibility Tests: No.

Rollout Plan

  • What is the rollout plan? No special rollout plan.
  • Does the order of deployment matter? Doesn't matter.
  • Is it safe to rollback? Does the order of rollback matter? Yes. The order doesn't matter.
  • Is there a kill switch to mitigate the impact immediately? No.

What changed?

  • Introduce data and data_encoding columns to the executions table of Cassandra. (NOTE: no data is written to Cassandra in this PR, it will be introduced in a separate PR)
  • Refactor persistence data type to replace transfer tasks, timer tasks, and replication tasks with tasksByCategory map of tasks
  • Remove cross cluster tasks logic from persistence layer

Why?

  • The new columns will be used to store data in executions table. A migration is required later to migrate transfer, timer and replication tasks data from their corresponding columns to data and data_encoding columns. The migration will be done in several phases:
    1. Dual writes: data is written in both transfer/timer/replication column and data, data_encoding columns
    2. Read from old columns
    3. Read from new columns
      Several other migrations will be initiated to migrate data from other columns of other types to these 2 columns.
  • The refactoring is to make it easier to introduce new categories of history tasks.
  • Cross cluster task is deprecated.
  • This change should only affect cadence-history

How did you test it?
Unit tests

Potential risks

Release notes

Documentation Changes

@Shaddoll Shaddoll force-pushed the persistence branch 4 times, most recently from bbc22a1 to 570d8ab Compare February 14, 2025 11:42
@Shaddoll Shaddoll changed the title WIP: Refactor peristence layer to support inserting history tasks of new categories Refactor peristence layer to support inserting history tasks of new categories Feb 14, 2025
@Shaddoll Shaddoll force-pushed the persistence branch 3 times, most recently from f8b12ac to 0f19761 Compare February 14, 2025 14:05
taylanisikdemir
taylanisikdemir reviewed Feb 18, 2025
View reviewed changes
common/persistence/data_manager_interfaces.go
Comment on lines +197 to +199
HistoryTaskCategoryIDTransfer = 1
HistoryTaskCategoryIDTimer = 2
HistoryTaskCategoryIDReplication = 3
Copy link
Member

Choose a reason for hiding this comment

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

category id to category type mapping is 1 to 1. Do we need the category type as a separate enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, at some place, I found I need to tell if the task is a scheduled task or immediate task.
And that requires another map to track which category is scheduled task and which category is immediate task.

common/persistence/nosql/nosql_execution_store.go Outdated
newWorkflow.TransferTasks, newWorkflow.CrossClusterTasks, newWorkflow.ReplicationTasks, newWorkflow.TimerTasks,
nil, nil, nil, nil,
newWorkflow.TasksByCategory,
map[persistence.HistoryTaskCategory][]*nosqlplugin.HistoryMigrationTask{},
Copy link
Member

Choose a reason for hiding this comment

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

HistoryMigrationTask is misleading because this type represents all kind of tasks. Did you name it Migration task because we are migrating to a new schema? Post-migration this won't make sense to readers. HistoryTaskV2 would be a more appropriate name I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm thinking we should migrate the history task data from transfer, timer and replication columns to task and task_encoding columns. And after migration, we can delete those 3 columns and get rid of this type. But HistoryTaskV2 is also a good name.

service/history/task/timer_active_task_executor_test.go
@@ -801,7 +801,7 @@ func (s *timerActiveTaskExecutorSuite) TestDecisionScheduleToStartTimeout_Normal
return req.UpdateWorkflowMutation.ExecutionInfo.DecisionAttempt == 1 &&
req.UpdateWorkflowMutation.ExecutionInfo.DecisionScheduleID == 4 &&
req.UpdateWorkflowMutation.ExecutionInfo.NextEventID == 4 && // transient decision
len(req.UpdateWorkflowMutation.TimerTasks) == 1 // another schedule to start timer
len(req.UpdateWorkflowMutation.TasksByCategory[persistence.HistoryTaskCategoryTimer]) == 1 // another schedule to start timer
Copy link
Member

Choose a reason for hiding this comment

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

can TasksByCategory be nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be nil, but in this test, it's not nil

@davidporter-id-au
Copy link
Member

do you mind adding a link or description as to what these changes are in service of?

@Shaddoll Shaddoll force-pushed the persistence branch 2 times, most recently from a11677a to 562ae40 Compare February 19, 2025 08:13
@Shaddoll Shaddoll merged commit f717213 into cadence-workflow:master Feb 20, 2025
23 checks passed
@Shaddoll Shaddoll deleted the persistence branch February 20, 2025 00:25
Assem-Uber pushed a commit that referenced this pull request Feb 20, 2025
@Shaddoll @Assem-Uber
Refactor peristence layer to support inserting history tasks of new c…
de3b186 
…ategories (#6671)

**Detailed Description**
Introduce data and data_encoding columns to the executions table of Cassandra. These field are used to replace other columns in `executions` table. We'll start by replacing the history task columns (`transfer`, `timer` and `replication`).

**Impact Analysis**
- **Backward Compatibility**: Yes. It only changes the schema, no data is written to the new columns in this PR.
- **Forward Compatibility**: Yes. It only changes the schema, no data is written to the new columns in this PR.

**Testing Plan**
- **Unit Tests**: Yes.
- **Persistence Tests**: No. The code implementation is not fully done in this PR.
- **Integration Tests**: No.
- **Compatibility Tests**: No.

**Rollout Plan**
- What is the rollout plan? No special rollout plan.
- Does the order of deployment matter? Doesn't matter.
- Is it safe to rollback? Does the order of rollback matter? Yes. The order doesn't matter.
- Is there a kill switch to mitigate the impact immediately? No.

<!-- Describe what has changed in this PR -->
**What changed?**
- Introduce `data` and `data_encoding` columns to the executions table of Cassandra. (NOTE: no data is written to Cassandra in this PR, it will be introduced in a separate PR)
- Refactor persistence data type to replace transfer tasks, timer tasks, and replication tasks with tasksByCategory map of tasks
- Remove cross cluster tasks logic from persistence layer

<!-- Tell your future self why have you made these changes -->
**Why?**
- The new columns will be used to store data in `executions` table. A migration is required later to migrate transfer, timer and replication tasks data from their corresponding columns to `data` and `data_encoding` columns. The migration will be done in several phases:
  1. Dual writes: data is written in both `transfer/timer/replication` column and `data`, `data_encoding` columns
  2. Read from old columns
  3. Read from new columns
Several other migrations will be initiated to migrate data from other columns of other types to these 2 columns.
- The refactoring is to make it easier to introduce new categories of history tasks.
- Cross cluster task is deprecated.
- This change should only affect cadence-history

<!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? -->
**How did you test it?**
Unit tests

<!-- Assuming the worst case, what can be broken when deploying this change to production? -->
**Potential risks**

<!-- Is it notable for release? e.g. schema updates, configuration or data migration required? If so, please mention it, and also update CHANGELOG.md -->
**Release notes**

<!-- Is there any documentation updates should be made for config, https://cadenceworkflow.io/docs/operation-guide/setup/ ? If so, please open an PR in https://github.com/cadence-workflow/cadence-docs -->
**Documentation Changes**
Assem-Uber pushed a commit that referenced this pull request Feb 20, 2025
@Shaddoll @Assem-Uber
Refactor peristence layer to support inserting history tasks of new c…
974f13a 
…ategories (#6671)

**Detailed Description**
Introduce data and data_encoding columns to the executions table of Cassandra. These field are used to replace other columns in `executions` table. We'll start by replacing the history task columns (`transfer`, `timer` and `replication`).

**Impact Analysis**
- **Backward Compatibility**: Yes. It only changes the schema, no data is written to the new columns in this PR.
- **Forward Compatibility**: Yes. It only changes the schema, no data is written to the new columns in this PR.

**Testing Plan**
- **Unit Tests**: Yes.
- **Persistence Tests**: No. The code implementation is not fully done in this PR.
- **Integration Tests**: No.
- **Compatibility Tests**: No.

**Rollout Plan**
- What is the rollout plan? No special rollout plan.
- Does the order of deployment matter? Doesn't matter.
- Is it safe to rollback? Does the order of rollback matter? Yes. The order doesn't matter.
- Is there a kill switch to mitigate the impact immediately? No.

<!-- Describe what has changed in this PR -->
**What changed?**
- Introduce `data` and `data_encoding` columns to the executions table of Cassandra. (NOTE: no data is written to Cassandra in this PR, it will be introduced in a separate PR)
- Refactor persistence data type to replace transfer tasks, timer tasks, and replication tasks with tasksByCategory map of tasks
- Remove cross cluster tasks logic from persistence layer

<!-- Tell your future self why have you made these changes -->
**Why?**
- The new columns will be used to store data in `executions` table. A migration is required later to migrate transfer, timer and replication tasks data from their corresponding columns to `data` and `data_encoding` columns. The migration will be done in several phases:
  1. Dual writes: data is written in both `transfer/timer/replication` column and `data`, `data_encoding` columns
  2. Read from old columns
  3. Read from new columns
Several other migrations will be initiated to migrate data from other columns of other types to these 2 columns.
- The refactoring is to make it easier to introduce new categories of history tasks.
- Cross cluster task is deprecated.
- This change should only affect cadence-history

<!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? -->
**How did you test it?**
Unit tests

<!-- Assuming the worst case, what can be broken when deploying this change to production? -->
**Potential risks**

<!-- Is it notable for release? e.g. schema updates, configuration or data migration required? If so, please mention it, and also update CHANGELOG.md -->
**Release notes**

<!-- Is there any documentation updates should be made for config, https://cadenceworkflow.io/docs/operation-guide/setup/ ? If so, please open an PR in https://github.com/cadence-workflow/cadence-docs -->
**Documentation Changes**
@Shaddoll Shaddoll mentioned this pull request Feb 24, 2025
Open
@ribaraka ribaraka mentioned this pull request Feb 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taylanisikdemir taylanisikdemir taylanisikdemir approved these changes

@neil-xie neil-xie Awaiting requested review from neil-xie neil-xie is a code owner

@davidporter-id-au davidporter-id-au Awaiting requested review from davidporter-id-au davidporter-id-au is a code owner

@Groxx Groxx Awaiting requested review from Groxx Groxx is a code owner

@shijiesheng shijiesheng Awaiting requested review from shijiesheng shijiesheng is a code owner

@jakobht jakobht Awaiting requested review from jakobht jakobht is a code owner

@3vilhamster 3vilhamster Awaiting requested review from 3vilhamster 3vilhamster is a code owner

@sankari165 sankari165 Awaiting requested review from sankari165 sankari165 is a code owner

@dkrotx dkrotx Awaiting requested review from dkrotx dkrotx is a code owner

@demirkayaender demirkayaender Awaiting requested review from demirkayaender demirkayaender is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Shaddoll @davidporter-id-au @taylanisikdemir