feat(cache): implement dynamic interface caching abstraction#4
Conversation
|
Warning Rate limit exceeded@Pyr33x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a Store abstraction for caching, implements memory and Redis Store backends, updates Cache to use Store, and surfaces cache type and TTL via configuration and proxy initialization; README and proxy code updated to select and pass TTL to the new cache constructor. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pkg/config/config.go (1)
49-51: Consider adding validation or documentation for valid cache types.The
Cachefield accepts any string but only"redis"and"memory"are valid values. Consider documenting valid values in a comment or adding validation to fail fast on invalid configuration.Proxy: Proxy{ Port: envy.GetString("PROXY_PORT", "1337"), - Cache: envy.GetString("PROXY_CACHE", "memory"), + Cache: envy.GetString("PROXY_CACHE", "memory"), // valid: "memory", "redis" TTL: envy.GetInt64("PROXY_TTL", 60), // in seconds },internal/cache/memory.go (1)
22-36: Lazy expiration means expired entries accumulate in memory.Entries are only evicted when accessed. With many keys that are never re-accessed, memory usage will grow unbounded. This is acceptable for low-traffic caches but consider documenting this limitation or adding periodic cleanup for production use.
internal/cache/cache.go (2)
46-53: Error case logged as "cache miss" may obscure actual failures.When
store.Getreturns an error (e.g., Redis connection failure), it's logged as a "MISS" which could mask infrastructure issues. Consider distinguishing between a store error and a cache miss.raw, err := c.store.Get(ctx, key) if err != nil { - c.logger.Info("cache miss", + c.logger.Warn("cache get error", zap.String("key", key), - zap.String("state", "MISS"), + zap.Error(err), ) return nil, false }
61-64: Unmarshal errors are silently swallowed.If cached data is corrupted or in an unexpected format, the error is not logged, making debugging difficult. Add logging for unmarshal failures.
var val CacheValue if err := json.Unmarshal(raw, &val); err != nil { + c.logger.Error("failed to unmarshal cached value", + zap.String("key", key), + zap.Error(err), + ) return nil, false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/cache/cache.go(6 hunks)internal/cache/memory.go(1 hunks)internal/cache/redis.go(1 hunks)internal/proxy/proxy.go(2 hunks)pkg/config/config.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/config/config.go (1)
internal/cache/cache.go (1)
Cache(20-24)
internal/proxy/proxy.go (5)
internal/cache/cache.go (3)
Store(14-18)Cache(20-24)NewCacheRepository(32-38)pkg/config/config.go (4)
Server(11-14)Proxy(20-24)New(42-67)Redis(30-33)internal/adapter/redis/redis.go (1)
New(16-31)internal/cache/redis.go (1)
NewRedisStore(14-16)internal/cache/memory.go (1)
NewMemoryStore(18-20)
internal/cache/memory.go (1)
internal/cache/cache.go (1)
Store(14-18)
🪛 GitHub Actions: 🔨 Lint
internal/proxy/proxy.go
[error] 34-34: GolangCI-Lint: ineffectual assignment to cacheStorage (ineffassign)
🪛 GitHub Check: lint (stable, ubuntu-latest)
internal/proxy/proxy.go
[failure] 34-34:
ineffectual assignment to cacheStorage (ineffassign)
🔇 Additional comments (6)
internal/cache/redis.go (1)
10-24: LGTM!The Redis store implementation correctly delegates to the Redis client and properly implements the
Storeinterface. TheGetandSetmethods handle byte conversion and TTL appropriately.pkg/config/config.go (1)
20-24: LGTM!The new
CacheandTTLfields extend the proxy configuration cleanly. The defaults (memory cache with 60-second TTL) are sensible.internal/proxy/proxy.go (1)
45-45: LGTM!The TTL conversion from
int64seconds totime.Durationis correctly applied when creating the cache repository.internal/cache/memory.go (1)
38-50: LGTM!The
Setmethod correctly computes expiration time and stores entries atomically usingsync.Map.Store.internal/cache/cache.go (2)
14-18: LGTM!The
Storeinterface is well-designed with a minimal, focused API that cleanly abstracts the underlying storage mechanism.
32-38: LGTM!The
NewCacheRepositoryconstructor and thePut/Clearmethods are well-implemented with appropriate error handling and logging.Also applies to: 69-94, 96-98
Summary by CodeRabbit
New Features
Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.