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

Fix bug that headers are removed in indexes for closed workflows #6234

Merged

Conversation

shijiesheng
Copy link
Contributor

What changed?

Cause:
For recording closed workflow task, headers are not appended into search attributes and thus the last closed event would remove the header in visibility.

Fix

  • move the logic to base transfer task
  • header append logic should exist in every record update (recordstart, upsert, recordclose)

Why?

From staging and production rolled out environment, we found header are not indexed correctly for closed workflows.

How did you test it?
unit test

Potential risks

Release notes

Documentation Changes

if attr == nil || attr.Header == nil {
return nil
}
headers := make(map[string][]byte)
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: this could be make(map[string][]byte, len(attr.Header.Fields))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 78.04878% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.95%. Comparing base (8c29b75) to head (acdd607).
Report is 12 commits behind head on master.

Files Patch % Lines
...ervice/history/task/transfer_task_executor_base.go 70.96% 4 Missing and 5 partials ⚠️
Additional details and impacted files
Files Coverage Δ
...vice/history/task/transfer_active_task_executor.go 67.05% <100.00%> (+0.06%) ⬆️
...ice/history/task/transfer_standby_task_executor.go 80.83% <100.00%> (+0.68%) ⬆️
...ervice/history/task/transfer_task_executor_base.go 80.40% <70.96%> (-0.04%) ⬇️

... and 13 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c29b75...acdd607. Read the comment docs.

Copy link
Member

@3vilhamster 3vilhamster left a comment

Choose a reason for hiding this comment

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

Looks ok, but you'll need to add more tests, since we want new code to have at least 75% coverage.

@shijiesheng shijiesheng merged commit c0cd4c5 into cadence-workflow:master Aug 22, 2024
19 checks passed
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.

4 participants