[WIP] replace in memory engine - #3
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request replaces the in-memory cache engine from go-freelru to otter. The change aims to swap out the underlying cache implementation while maintaining the same API surface for the cachestore backend.
Changes:
- Replaced
go-freelrulibrary withotterfor in-memory caching - Removed custom sharding logic and hash function (xxh3) in favor of otter's built-in implementation
- Modified TTL handling to work with otter's API (using 1-year TTL to simulate "no expiry")
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| memcache.go | Replaced freelru cache with otter cache, updated all cache operations to use otter's API, and adjusted TTL handling logic |
| go.mod | Updated dependencies to replace go-freelru and zeebo/xxh3 with maypok86/otter and its transitive dependencies |
| go.sum | Updated dependency checksums to reflect the new otter library and removal of old dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| capacity := uint32(float64(size) * 1.25) | ||
| lru, err := freelru.NewShardedWithSize[string, V](shards, size, capacity, hashStringXXH3) | ||
| cache, err := otter.MustBuilder[string, V](int(size)).WithVariableTTL().Build() |
There was a problem hiding this comment.
The use of MustBuilder is inconsistent with error handling. MustBuilder typically panics on error rather than returning an error, making the subsequent error check on line 30 ineffective. Consider using Builder instead of MustBuilder if you want to handle errors gracefully, or remove the error check if MustBuilder is intentional (though this would panic on failure).
| cache, err := otter.MustBuilder[string, V](int(size)).WithVariableTTL().Build() | |
| cache, err := otter.Builder[string, V](int(size)).WithVariableTTL().Build() |
| return m.cache.Has(key), nil | ||
| } | ||
|
|
||
| func (m *MemLRU[V]) Set(ctx context.Context, key string, value V) error { |
There was a problem hiding this comment.
The comment states that TTL=0 means the key will not expire and will only be evicted by LRU. However, with the new implementation (lines 200-202), TTL=0 is converted to a 1-year expiration (noExpireTTL). This changes the behavior from true "no expiry" to "expires in 1 year", which contradicts the comment. The comment should be updated to reflect the actual behavior with the new otter cache implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m.cache.Set(key, value) | ||
| if ttl > 0 { | ||
| m.lru.AddWithLifetime(key, value, ttl) | ||
| } else { | ||
| m.lru.Add(key, value) | ||
| m.cache.SetExpiresAfter(key, ttl) | ||
| } |
There was a problem hiding this comment.
The TTL/expiry implementation may not behave correctly. The cache is initialized with ExpiryCalculator: otter.ExpiryWriting[string, V](0) which sets a default expiry of 0. Then in setKeyValue, all keys are set with Set() first, and only if ttl > 0 is SetExpiresAfter() called.
The previous implementation used two distinct methods: AddWithLifetime() for keys with expiry and Add() for keys without expiry. The new implementation should verify that:
- When
ttl == 0, keys are stored without expiry and will only be evicted by the LRU policy - When
ttl > 0, callingSet()followed bySetExpiresAfter()correctly applies the TTL
Consider using otter's SetWithTTL() method if available, or ensure the current two-step approach (Set then SetExpiresAfter) is the recommended pattern for otter cache.
There was a problem hiding this comment.
This works fine. Added TestTTLExpiryCorrectness and TestTTLZeroDoesNotExpire.
| cache, err := otter.New[string, V](&otter.Options[string, V]{ | ||
| MaximumSize: int(size), | ||
| ExpiryCalculator: otter.ExpiryWriting[string, V](0), // enable expiry, actual TTL set per-key | ||
| }) |
There was a problem hiding this comment.
The cache initialization has been simplified by removing the shard calculation logic. The previous implementation calculated optimal shards based on CPU count and size. While otter likely handles this internally, it would be good to verify that otter's internal sharding provides similar or better performance characteristics, especially for high-concurrency scenarios.
The removed logic was:
- maxShards = NumCPU * 16
- minShards = max(1, size/NumCPU)
- Used minShards if size <= maxShards*2, else maxShards
Consider documenting whether otter's internal sharding is sufficient or if any configuration is needed for optimal concurrency.
| m.cache.Set(key, value) | ||
| if ttl > 0 { | ||
| m.lru.AddWithLifetime(key, value, ttl) | ||
| } else { | ||
| m.lru.Add(key, value) | ||
| m.cache.SetExpiresAfter(key, ttl) |
There was a problem hiding this comment.
There's a potential race condition in the TTL setting logic. The key is first set with Set(key, value) on line 191, and then the expiry is set with SetExpiresAfter(key, ttl) on line 193. Between these two operations, another goroutine could read the key and get a value without the intended expiry time set.
Consider using an atomic operation or a single method call to set both the key-value pair and its expiry together, if otter provides such an API.
There was a problem hiding this comment.
Technically valid, but not real-world problem. I added TestConcurrentSetWithTTLAndGet. Nothing to worry about.
83d5e8d to
8481724
Compare
This PR replaces in-memory engine from
go-freelrutootter/v2