Add hot-swap capability between count-based and size-based cache modes #6809
Conversation
| } | ||
| cache.sizeByKey = make(map[interface{}]uint64, opts.InitialCapacity) | ||
| if opts.IsSizeBased == nil { | ||
| cache.isSizeBased = dynamicproperties.GetBoolPropertyFn(false) |
There was a problem hiding this comment.
I prefer to panic in this case. I'd like to have the behavior of cache to be completely managed by dynamic config. These implicit logic can make it hard to debug
There was a problem hiding this comment.
I want to add the panic logic later once we add feature flags for all the cache usages. For now, existing usage will be default to count based.
There was a problem hiding this comment.
I'd avoid panic'ing in library level and instead return an error for missing mandatory options
There was a problem hiding this comment.
Thanks, I will add the error returning and make this into a mandatory flag once we add the feature flags for all other cache usage.
| } | ||
| cache.sizeByKey = make(map[interface{}]uint64, opts.InitialCapacity) | ||
| if opts.IsSizeBased == nil { | ||
| cache.isSizeBased = dynamicproperties.GetBoolPropertyFn(false) |
There was a problem hiding this comment.
I'd avoid panic'ing in library level and instead return an error for missing mandatory options
| if c.isSizeBased() { | ||
| if valueSize > uint64(c.maxSize()) { | ||
| // value is too big to be cached, we also don't want to evict everyone else | ||
| return nil, ErrEntryTooBig |
There was a problem hiding this comment.
This is a good idea but we may need to update callers to work without caching for big workflow executions. Otherwise they will be stuck because caching the workflow execution is mandatory to process the workflow.
There was a problem hiding this comment.
Got it, this is similar to when the cache is full of pinned elements and can't evict or add new entries. One way I can think of is to avoid that is by setting a minimum cache size that fits any workflow execution—for example, 50MB if the limit is 30MB.
I'll add a comment to cover this edge case in the cache usage logic.
What changed?
This PR introduces a hot-swap capability between count-based and size-based cache modes in the LRU cache implementation. The key changes include:
Why?
The current implementation requires a complete cache restart when switching between count-based and size-based modes, which:
This change enables smooth transitions between modes while maintaining cache contents, improving system stability and performance.
How did you test it?
I tested the logic with unit test and local testing.
Potential risks
Since it's the base cache, it could break every part that used this cache if there was a bug hidden. We need to rollback immediately if such things happens.
Release notes
Documentation Changes