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

Handle replicate workflow state with exist workflow data #4617

Merged
merged 9 commits into from
Jul 13, 2023
Merged

Handle replicate workflow state with exist workflow data #4617

merged 9 commits into from
Jul 13, 2023

Conversation

yux0
Copy link
Contributor

@yux0 yux0 commented Jul 12, 2023

What changed?
Handle replicate workflow state with exist workflow data.

Why?
During migration interruption, some workflow data might be partially migrated. This case is not handle in replicate workflow state.

How did you test it?
Update current unit test.

A manual test will be perform:

  1. Connect two cluster.
  2. Start a long running workflow.
  3. Disconnect two cluster.
  4. Terminate the old workflow and start a new long running workflow.
  5. Force replication open workflow.
  6. Force replication closed workflow.

Potential risks

Is hotfix candidate?
Yes.

@yux0 yux0 requested a review from wxing1292 July 12, 2023 04:45
@yux0 yux0 requested a review from a team as a code owner July 12, 2023 04:45
yux0
yux0 commented Jul 12, 2023
View reviewed changes
service/history/ndc/history_replicator.go
lastVersionHistory, err := versionhistory.GetLastVersionHistoryItem(currentVersionHistory)
if err != nil {
return err
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if the local last version history and remote last version history are identical

yux0
yux0 commented Jul 12, 2023
View reviewed changes
service/history/replication/executable_workflow_state_task.go Outdated
); resendErr != nil {
return err
}
return nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.Execute()

wxing1292
wxing1292 approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@wxing1292 wxing1292 left a comment

Choose a reason for hiding this comment

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

plz address the comments about API idempotency then land the PR

wxing1292
wxing1292 approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@wxing1292 wxing1292 left a comment

Choose a reason for hiding this comment

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

LGTM

yycptt
yycptt approved these changes Jul 13, 2023
View reviewed changes
service/history/replication/task_executor.go
Copy link
Member

Choose a reason for hiding this comment

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

is this a different code path than executable_workflow_state_task_test.go for replication task execution? Is there a plan for consolidating them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file will be deprecated once we move to streaming replication.

@yux0 yux0 enabled auto-merge (squash) July 13, 2023 02:25
@yux0 yux0 merged commit 6e811dd into temporalio:master Jul 13, 2023
wxing1292 pushed a commit that referenced this pull request Jul 13, 2023
@yux0 @wxing1292
Handle replicate workflow state with exist workflow data (#4617)
dd940dc 
<!-- Describe what has changed in this PR -->
**What changed?**
Handle replicate workflow state with exist workflow data.

<!-- Tell your future self why have you made these changes -->
**Why?**
During migration interruption, some workflow data might be partially
migrated. This case is not handle in replicate workflow state.

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

A manual test will be perform: 
1. Connect two cluster.
2. Start a long running workflow.
3. Disconnect two cluster.
4. Terminate the old workflow and start a new long running workflow.
5. Force replication open workflow.
6. Force replication closed workflow.

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


<!-- Is this PR a hotfix candidate or require that a notification be
sent to the broader community? (Yes/No) -->
**Is hotfix candidate?**
Yes.
wxing1292 pushed a commit that referenced this pull request Jul 14, 2023
@yux0 @wxing1292
Handle replicate workflow state with exist workflow data (#4617)
1d141df 
<!-- Describe what has changed in this PR -->
**What changed?**
Handle replicate workflow state with exist workflow data.

<!-- Tell your future self why have you made these changes -->
**Why?**
During migration interruption, some workflow data might be partially
migrated. This case is not handle in replicate workflow state.

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

A manual test will be perform: 
1. Connect two cluster.
2. Start a long running workflow.
3. Disconnect two cluster.
4. Terminate the old workflow and start a new long running workflow.
5. Force replication open workflow.
6. Force replication closed workflow.

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


<!-- Is this PR a hotfix candidate or require that a notification be
sent to the broader community? (Yes/No) -->
**Is hotfix candidate?**
Yes.
dnr pushed a commit that referenced this pull request Jul 21, 2023
@yux0 @dnr
Handle replicate workflow state with exist workflow data (#4617)
f498156 
<!-- Describe what has changed in this PR -->
**What changed?**
Handle replicate workflow state with exist workflow data.

<!-- Tell your future self why have you made these changes -->
**Why?**
During migration interruption, some workflow data might be partially
migrated. This case is not handle in replicate workflow state.

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

A manual test will be perform: 
1. Connect two cluster.
2. Start a long running workflow.
3. Disconnect two cluster.
4. Terminate the old workflow and start a new long running workflow.
5. Force replication open workflow.
6. Force replication closed workflow.

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


<!-- Is this PR a hotfix candidate or require that a notification be
sent to the broader community? (Yes/No) -->
**Is hotfix candidate?**
Yes.
dnr pushed a commit that referenced this pull request Jul 21, 2023
@yux0 @dnr
Handle replicate workflow state with exist workflow data (#4617)
bcbb3b9 
<!-- Describe what has changed in this PR -->
**What changed?**
Handle replicate workflow state with exist workflow data.

<!-- Tell your future self why have you made these changes -->
**Why?**
During migration interruption, some workflow data might be partially
migrated. This case is not handle in replicate workflow state.

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

A manual test will be perform: 
1. Connect two cluster.
2. Start a long running workflow.
3. Disconnect two cluster.
4. Terminate the old workflow and start a new long running workflow.
5. Force replication open workflow.
6. Force replication closed workflow.

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


<!-- Is this PR a hotfix candidate or require that a notification be
sent to the broader community? (Yes/No) -->
**Is hotfix candidate?**
Yes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yycptt yycptt yycptt approved these changes

@wxing1292 wxing1292 wxing1292 approved these changes

Assignees
No one assigned
Labels
release/1.21.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yux0 @yycptt @wxing1292