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 dynamic config for namespace refresh interval #2766

Merged
merged 6 commits into from
Apr 26, 2022

Conversation

meiliang86
Copy link
Contributor

What changed?
Add dynamic config for namespace refresh interval

Why?
To reduce wait time for namespace promotion and failover.

How did you test it?
Local tests.

Potential risks

Is hotfix candidate?

@meiliang86 meiliang86 requested a review from a team as a code owner April 25, 2022 20:55
@meiliang86 meiliang86 marked this pull request as draft April 25, 2022 20:59
@dnr
Copy link
Member

dnr commented Apr 25, 2022

There's a manual force refresh call, although it's not exposed as a public api. Maybe we should expose that and call it when appropriate?

@meiliang86 meiliang86 marked this pull request as ready for review April 25, 2022 23:34
@meiliang86
Copy link
Contributor Author

meiliang86 commented Apr 25, 2022

There's a manual force refresh call, although it's not exposed as a public api. Maybe we should expose that and call it when appropriate?

No. It would be hard to call into refresh because each service instance has its own cache.

// Put timer events on our channel so we can select on just one below.
go func() {
for range timer.C {
timer := time.NewTicker(r.refreshInterval())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: how about use a new timer and reset it after it fires

Copy link
Contributor Author

@meiliang86 meiliang86 Apr 26, 2022

Choose a reason for hiding this comment

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

Do you mean instead of doing
timer.Stop()
timer = time.NewTicker(r.refreshInterval())
I do timer.Reset?

@@ -196,7 +197,7 @@ func (e *executableImpl) HandleErr(err error) (retErr error) {
// TODO remove this error check special case
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know :)
@yycptt Should I remove it or do you want to do it later in your PR?

Copy link
Member

Choose a reason for hiding this comment

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

I will do it later after merging active & standby queue processor.

@@ -196,7 +197,7 @@ func (e *executableImpl) HandleErr(err error) (retErr error) {
// TODO remove this error check special case
Copy link
Member

Choose a reason for hiding this comment

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

I will do it later after merging active & standby queue processor.

@meiliang86 meiliang86 enabled auto-merge (squash) April 26, 2022 20:00
@meiliang86 meiliang86 merged commit 631e27d into temporalio:master Apr 26, 2022
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