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

Improve cross cluster components shutdown logic #4662

Merged
merged 3 commits into from
Dec 1, 2021

Conversation

yycptt
Copy link
Contributor

@yycptt yycptt commented Nov 30, 2021

What changed?

  • Improve cross cluster task processor and fetcher shutdown logic so that on shutdown, they won't block on pending requests. This is done by cancelling the context used for making calls.
  • Changes in the cross-cluster task processor is not required since we currently shut down the task fetcher first and the fetcher will fail all task processors' requests to it. But if in the future we shutdown task processors first, without this fix, they will be blocked since they're waiting for results from the task fetcher.

Why?

  • Improve shutdown logic

How did you test it?

  • Tested locally

Potential risks

Release notes

Documentation Changes

@yycptt yycptt requested review from demirkayaender and a team November 30, 2021 19:17
@coveralls
Copy link

coveralls commented Nov 30, 2021

Pull Request Test Coverage Report for Build f4863a24-7d93-4925-8cb1-90f21d64c936

  • 29 of 39 (74.36%) changed or added relevant lines in 2 files are covered.
  • 42 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.02%) to 56.934%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/history/task/cross_cluster_task_processor.go 14 24 58.33%
Files with Coverage Reduction New Missed Lines %
common/types/shared.go 1 26.8%
client/history/client.go 2 39.33%
client/history/metricClient.go 2 46.37%
common/task/weightedRoundRobinTaskScheduler.go 2 89.12%
service/history/handler.go 2 48.56%
service/history/task/transfer_active_task_executor.go 2 71.47%
service/matching/taskListManager.go 2 74.19%
service/history/task/fetcher.go 3 86.67%
common/types/mapper/thrift/shared.go 4 63.2%
service/frontend/workflowHandler.go 4 59.28%
Totals Coverage Status
Change from base Build 4259fd31-5d91-4273-85c1-b4b2436b18ac: 0.02%
Covered Lines: 82772
Relevant Lines: 145382

💛 - Coveralls

Comment on lines 317 to 319
if p.ctx.Err() != nil {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

would this be better above, in the loop? or is that unsafe to interrupt? (though if anything under it checks the context, it may still be interrupted)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I don't have a strong opinion on this. Moving it into the for loop also works and probably can save us some iterations during the shutdown.

I was thinking the for loop is already non-blocking once the context is cancelled on shutdown, and we just need one check before sending the response back instead of checking it in each iteration.

@Groxx
Copy link
Contributor

Groxx commented Nov 30, 2021

I don't follow the set of changes as a whole, but reducing context.Backgrounds is very very much a good thing. 👍 👍

@yycptt yycptt merged commit c894177 into cadence-workflow:master Dec 1, 2021
@yycptt yycptt deleted the improve-xcluster-shutdown branch December 1, 2021 23:55
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