[CRE] Refactor caching (fka fallback) OrgResolver - #2342
Conversation
|
0fea694 to
036c1a8
Compare
bolekk
changed the title
|
Can we find a way to evolve without breaking the API?
|
| DefaultRefreshInterval = 10 * time.Minute | ||
| DefaultRefreshWorkers = 4 |
There was a problem hiding this comment.
Do these need to be exported?
| func NewInMemoryCache() *InMemoryCache { | ||
| return &InMemoryCache{} | ||
| } |
There was a problem hiding this comment.
Could exclude since it's not currently necessary and we may want to use a different signature in the future.
| func NewInMemoryCache() *InMemoryCache { | |
| return &InMemoryCache{} | |
| } |
| logger log.SugaredLogger | ||
|
|
||
| refreshCh chan refreshJob | ||
| refreshing sync.Map // owner (string) -> struct{}{} while queued or being refreshed |
There was a problem hiding this comment.
Why use a sync.Map in this case?
| } | ||
|
|
||
| type InMemoryCache struct { | ||
| entries sync.Map // owner (string) -> CacheEntry |
There was a problem hiding this comment.
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.
| // cached it is trusted indefinitely and returned immediately, without ever | ||
| // blocking on a call to the underlying resolver. To guard against a bad | ||
| // entry becoming permanent (e.g. from a resolver bug or a transient | ||
| // corruption), an entry older than RefreshInterval also queues a |
There was a problem hiding this comment.
this will be improved by a batch API on the linking server
| // At most one refresh per owner is queued/running at a time. If the queue is | ||
| // full, the request is dropped; it will be retried on a later stale cache | ||
| // hit. | ||
| func (c *CachingResolver) queueRefresh(owner string, previous CacheEntry) { |
There was a problem hiding this comment.
I wonder if the additional complexity is worth it. If we just lazy load the values we can save on populating a queue, and once we get the batch API on the server side, we can warm the cache completely and basically drop TPS against the org resolver from this node to a call every few minutes (ofc we will need to paginate)
There was a problem hiding this comment.
Hmm with the batch API, are you suggesting that we could periodically fetch ALL entries in a single call?
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).