Added exponential retry to the domain cache #6676
Conversation
jakobht
force-pushed
the
Add-exponential-retry-to-the-domain-cache
branch
from
ac53f8f to
7991588
Compare
| } | ||
| err := c.refreshDomains() | ||
| if err != nil { | ||
| c.logger.Error("Error refreshing domain cache", tag.Error(err)) |
There was a problem hiding this comment.
We could consider logging fatal here? Since we already retryed quite a lot, and we still failed.
There was a problem hiding this comment.
Let's leave it like this. It may have failed due to persistence rps limits and in those cases we don't want to get into crash loop. Ideally fetching domains shouldn't be subject to the same limits. @davidporter-id-au will look into granular rate limits area.
jakobht
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
common/util.go
Outdated
| @@ -88,6 +88,10 @@ const ( | |||
| taskCompleterMaxInterval = 10 * time.Second | |||
| taskCompleterExpirationInterval = 5 * time.Minute | |||
|
|
|||
| domainCacheInitialInterval = 1 * time.Second | |||
| domainCacheMaxInterval = 20 * time.Second | |||
There was a problem hiding this comment.
Can I request we make this lower, like 5 seconds I think? I think there's the problem that this logic is smeared around in a few places (at least
cadence/common/cache/domainCache.go
Line 63 in e36c818
cadence/service/history/task/task.go
Line 311 in e36c818
I agree it's the subject of unnecessary load, but the domain cache (in)consistency across hosts during transfer tasks is a subject of some pretty difficult-to-figure out bugs during failover (because a child workflow might go to a different history host, with a different view of which region is active, and if hte domain cache doesn't update within a reasonable time-period we have no clear resolution strategy)
There was a problem hiding this comment.
Sure - that's also a good justification for looking into this more. Since in the end, there is no guarantee that this will succeed in any reasonable time when it has already started failing.
jakobht
force-pushed
the
Add-exponential-retry-to-the-domain-cache
branch
from
7991588 to
64f975f
Compare
jakobht
force-pushed
the
Add-exponential-retry-to-the-domain-cache
branch
from
a8bd03f to
fb5586b
Compare
What changed? Added a proper retry policy to the domain cache so it does not just retry every second Why? The load from retrying every second can cause too much load on the database. How did you test it? Unit test Potential risks Might cause domain data to be out of date, but we are already in that situation when it's failing. Release notes Documentation Changes
What changed? Added a proper retry policy to the domain cache so it does not just retry every second Why? The load from retrying every second can cause too much load on the database. How did you test it? Unit test Potential risks Might cause domain data to be out of date, but we are already in that situation when it's failing. Release notes Documentation Changes
What changed?
Added a proper retry policy to the domain cache so it does not just retry every second
Why?
The load from retrying every second can cause too much load on the database.
How did you test it?
Unit test
Potential risks
Might cause domain data to be out of date, but we are already in that situation when it's failing.
Release notes
Documentation Changes