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 lower priority for replication tasks #3870

Merged
merged 6 commits into from
Jan 31, 2023

Conversation

pdoerner
Copy link
Contributor

@pdoerner pdoerner commented Jan 30, 2023

What changed?
Added a new rate limiter priority for preemptable tasks which is lower than other background tasks (currently the lowest priority)
Applied this new priority to replication tasks

Why?
Replication tasks should have lower priority than other tasks because it can hog resources and impact active namespaces in a cell. This will allow us to throttle replication tasks independently from other background tasks.

How did you test it?
Update unit test

Potential risks
No risks

Is hotfix candidate?
No

@pdoerner pdoerner requested a review from yycptt January 30, 2023 22:47
@pdoerner pdoerner marked this pull request as ready for review January 30, 2023 23:00
@pdoerner pdoerner requested a review from a team as a code owner January 30, 2023 23:00
@yycptt yycptt requested a review from yux0 January 31, 2023 04:24
Copy link
Contributor

@yux0 yux0 left a comment

Choose a reason for hiding this comment

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

LGTM. Should we use CallerTypePreemptable for archival queue?

@yycptt
Copy link
Member

yycptt commented Jan 31, 2023

LGTM. Should we use CallerTypePreemptable for archival queue?

Yeah, the plan is to use the new level/caller type for replication and all queues other than transfer/timer/visibility (@pdoerner Can you create a task for it). But that involves more changes: e.g right now task executable assumes all tasks are background, we need a ways to specify different caller type for different task categories. Same for the implementation on task loading.

Given that the main motivation for this work is to resolve the issues we saw during namespace migration, I don't want to rush ^ changes for 1.20.

@pdoerner pdoerner merged commit 7e3daf1 into temporalio:master Jan 31, 2023
@pdoerner pdoerner deleted the lower-replication-priority branch January 31, 2023 19:26
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.

3 participants