Skip to content

[CRE] Refactor caching (fka fallback) OrgResolver - #2342

Open
bolekk wants to merge 1 commit into
mainfrom
org_cache_2
Open

[CRE] Refactor caching (fka fallback) OrgResolver#2342
bolekk wants to merge 1 commit into
mainfrom
org_cache_2

Conversation

@bolekk

@bolekk bolekk commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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).

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

⚠️ API Diff Results - github.com/smartcontractkit/chainlink-common

⚠️ Breaking Changes (2)

pkg/services/orgresolver (2)
  • NewOrgResolverWithFallback — 🗑️ Removed

  • OrgResolverFallback — 🗑️ Removed

✅ Compatible Changes (9)

pkg/services/orgresolver (9)
  • Cache — ➕ Added

  • CacheEntry — ➕ Added

  • CachingResolver — ➕ Added

  • CachingResolverConfig — ➕ Added

  • DefaultRefreshInterval — ➕ Added

  • DefaultRefreshWorkers — ➕ Added

  • InMemoryCache — ➕ Added

  • NewCachingResolver — ➕ Added

  • NewInMemoryCache — ➕ Added


📄 View full apidiff report

@bolekk
bolekk force-pushed the org_cache_2 branch 2 times, most recently from 0fea694 to 036c1a8 Compare August 26, 2026 02:26
@bolekk bolekk changed the title [CRE] Caching OrgResolver [CRE] Refactor caching (fka fallback) OrgResolver Aug 26, 2026
@bolekk
bolekk marked this pull request as ready for review August 26, 2026 03:11
@bolekk
bolekk requested a review from a team as a code owner August 26, 2026 03:11
@bolekk
bolekk requested a review from jmank88 August 26, 2026 04:43
@jmank88

jmank88 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Can we find a way to evolve without breaking the API?

⚠️ Breaking Changes (2)
pkg/services/orgresolver (2)

  • NewOrgResolverWithFallback — 🗑️ Removed

  • OrgResolverFallback — 🗑️ Removed

Comment on lines +30 to +31
DefaultRefreshInterval = 10 * time.Minute
DefaultRefreshWorkers = 4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do these need to be exported?

Comment on lines +82 to +84
func NewInMemoryCache() *InMemoryCache {
return &InMemoryCache{}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could exclude since it's not currently necessary and we may want to use a different signature in the future.

Suggested change
func NewInMemoryCache() *InMemoryCache {
return &InMemoryCache{}
}

logger log.SugaredLogger

refreshCh chan refreshJob
refreshing sync.Map // owner (string) -> struct{}{} while queued or being refreshed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why use a sync.Map in this case?

}

type InMemoryCache struct {
entries sync.Map // owner (string) -> CacheEntry

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why a sync.Map instead of a regular map and mutex?

The Map type is specialized. Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content.

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

https://pkg.go.dev/sync#Map

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.

2 participants