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

Reduce default namespace cache refresh interval to 2s #7209

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

pdoerner
Copy link
Contributor

What changed?

Reduced the default namespace cache refresh interval (dynamic config system.namespaceCacheRefreshInterval) from 10s -> 2s

Why?

  1. To increase responsiveness of the system
  2. Migration handover wait was reduced to 10s and the cache refresh interval should be shorter than this value.

How did you test it?

Existing tests

Potential risks

Additional load on persistence from increased calls to refresh namespaces in the cache.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@pdoerner pdoerner requested a review from a team as a code owner January 31, 2025 22:38
@pdoerner pdoerner requested review from yux0 and yycptt January 31, 2025 22:40
@@ -42,7 +42,7 @@ import (
const (
WorkflowName = "temporal-sys-reclaim-namespace-resources-workflow"

namespaceCacheRefreshDelay = 11 * time.Second
namespaceCacheRefreshDelay = 3 * time.Second
Copy link
Member

@yycptt yycptt Jan 31, 2025

Choose a reason for hiding this comment

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

🤦

Let's not change it? In production we are using 2s and 11s combination and thing are fine. 2s and 3s I am not sure. Relying on this delay to make sure no new workflows are created is kinda dangerous.

cc @alexshtin WDYT?

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, no strong opinion for me. Just did a quick search to try to find anywhere else we were redefining it as a constant.

Copy link
Member

@alexshtin alexshtin Jan 31, 2025

Choose a reason for hiding this comment

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

I didn't realize that we have DC for this interval. This WF should read DC, not have its own const. I will change it. Revert it to 11 for now.

Copy link
Member

Choose a reason for hiding this comment

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

pdoerner and others added 2 commits February 3, 2025 10:37
@pdoerner pdoerner enabled auto-merge (squash) February 3, 2025 18:38
@pdoerner pdoerner merged commit 020195d into main Feb 3, 2025
50 checks passed
@pdoerner pdoerner deleted the reduce-default-ns-cache-refresh branch February 3, 2025 18:58
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.

None yet

5 participants