[CRE] Refactor caching (fka fallback) OrgResolver - #2342
Conversation
✅ API Diff Results -
|
0fea694 to
036c1a8
Compare
bolekk
changed the title
cea4809 to
61a290e
Compare
|
Can we find a way to evolve without breaking the API?
|
|
@jmank88 I don't think it makes sense to keep old APIs since I renamed the file and types. It's not "fallback" any more. |
Compilation compatibility trumps undesirable naming though. It doesn't make sense to arbitrarily break unrelated things because the names don't make sense. |
|
@jmank88 I disagree. It's only used in a single place in core and the old name is now confusing. NewOrgResolverWithFallback - there's no fallback, cached values are the new source of truth. |
Having one callsite from one module does not mean that there is only one thing to worry about. There are multiple versions of core that may need patch releases that could encounter this. There are also manys modules that import core and could be affected by this. Coincidentally, I am currently blocked from acting on basic dependabot security alerts in Aptos, because a trivial dependency bump is not possible, since we deleted a bunch of API that is superficially not related. |
|
@jmank88 to me this tradeoff is not worth it. But to move this PR forward, I brought back the old fallback resolver and marked it as deprecated - is that OK? |
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-time.After(refreshOwnerDelay): |
There was a problem hiding this comment.
should this be i*refreshOwnerDelay? if not, why have refreshOwnerDelay? the refreshLoop already waits for the refreshInterval.
There was a problem hiding this comment.
Every 10 mins we cycle over all owners with 100ms gaps between them.
There was a problem hiding this comment.
This is to smooth out the spike in traffic on eery refreshInterval. It will go away with batching anyway.
|
|
||
| // The loop must outlive the Start call, so it gets its own context, | ||
| // cancelled explicitly by Close rather than inherited from ctx. | ||
| loopCtx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
/nit i believe you could use the stopCh context helper here instead of a raw context.Background()
patrickhuie19
left a comment
patrickhuie19
left a comment
There was a problem hiding this comment.
mostly lgtm with one q on owner specific delay
3 participants
Change the behavior to always depend on the cached value (if available) to reduce the volume of calls. Only refresh periodically in the background (to recover from potential bad data).
Core counterpart: smartcontractkit/chainlink#23485