Implement default memory cache store #9
Conversation
|
Warning Rate limit exceeded@nDmitry has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 11 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 (3)
WalkthroughAdds an in-memory cache implementation and makes Redis optional: if REDIS_HOST is unset the app uses an in-memory cache; otherwise it uses Redis. Makefile, Docker Compose, README, main initialization, cache package, and tests updated accordingly. 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 |
nDmitry
force-pushed
the
memory-cache
branch
from
ddd9ce8 to
d0284d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/tgfeed/main.go (1)
65-79: Memory cacheClose()is not called on shutdown.The Redis client has
defer redisClient.Close()(line 77), but when using the memory cache,Close()is never called. While this is less critical for an in-memory cache (memory is released on process exit anyway), it would be more consistent to defer the close for both implementations.🔎 Proposed fix for consistent cleanup
var c cache.Cache if redisHost == "" { c = cache.NewMemoryClient() } else { redisClient, err := cache.NewRedisClient(ctx, fmt.Sprintf("%s:6379", redisHost)) if err != nil { logger.Error("Failed to connect to Redis", "error", err) os.Exit(1) } - defer redisClient.Close() c = redisClient } + + defer c.Close()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(1 hunks)README.md(2 hunks)cmd/tgfeed/main.go(1 hunks)compose.yaml(2 hunks)internal/cache/cache.go(1 hunks)internal/cache/memory.go(1 hunks)internal/cache/memory_test.go(1 hunks)internal/cache/redis.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/cache/redis.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cache/memory_test.go (2)
internal/cache/memory.go (2)
MemoryCache(11-14)NewMemoryClient(17-21)internal/cache/cache.go (1)
ErrCacheMiss(10-10)
internal/cache/memory.go (1)
internal/cache/cache.go (1)
ErrCacheMiss(10-10)
🪛 checkmake (0.2.2)
Makefile
[warning] 2-2: Missing required phony target "all"
(minphony)
[warning] 2-2: Missing required phony target "clean"
(minphony)
🪛 LanguageTool
README.md
[style] ~68-~68: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...comment some config values there if you want to keep cache in Redis. Otherwise it will ...
(REP_WANT_TO_VB)
🔇 Additional comments (6)
internal/cache/cache.go (1)
9-10: LGTM! Good use of a package-level sentinel error.Centralizing
ErrCacheMissin the cache package allows both memory and Redis implementations to share the same error type, enabling consistent error handling by callers usingerrors.Is().README.md (1)
11-11: Documentation updates look good.The changes accurately reflect the new default behavior where caching is done in RAM unless Redis is explicitly configured.
Also applies to: 68-68
Makefile (1)
10-11: Good addition of the race detection target.This is particularly valuable given the new concurrent memory cache implementation with goroutines and mutex usage.
compose.yaml (1)
8-9: Configuration changes align well with the new default behavior.The comments are clear and provide users with easy instructions to re-enable Redis if needed.
Also applies to: 27-29, 31-50
internal/cache/memory_test.go (1)
204-239: Test logic is correct, but there's a subtle timing issue.The
defer cancel()at line 206 is redundant sincecancel()is explicitly called at line 232. This is harmless but could be cleaned up.More importantly, the test verifies that context cancellation stops the TTL goroutine, but note that this behavior means the cache entry will never be evicted (as flagged in memory.go review). Ensure this is the intended behavior.
cmd/tgfeed/main.go (1)
67-68: Good default behavior selection.The logic correctly defaults to the in-memory cache when
REDIS_HOSTis not configured, aligning with the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/cache/memory.go (1)
47-59: Context cancellation still prevents TTL expiration.This issue was previously flagged: when the context is cancelled (lines 56-57), the eviction goroutine exits without deleting the cache entry. If request contexts are passed to
Set, cancelled requests will leave entries in memory indefinitely, causing unbounded growth.The previously suggested solutions remain valid—either remove the context cancellation branch so the timer always completes, or use
context.Background()for the eviction goroutine.
🧹 Nitpick comments (2)
internal/cache/memory.go (2)
16-21: Consider extracting the initial capacity as a package constant.The hardcoded capacity of
100works fine but could be made more maintainable.🔎 Proposed refactor
+const defaultInitialCapacity = 100 + // NewMemoryClient creates a new cache client func NewMemoryClient() *MemoryCache { - cache := make(map[string][]byte, 100) + cache := make(map[string][]byte, defaultInitialCapacity) return &MemoryCache{cache: cache} }
64-71: Data race fixed, but eviction goroutines aren't stopped.Good fix—the mutex is now properly acquired before clearing the cache, addressing the previous data race concern.
However, eviction goroutines spawned by
Setwill continue running and may attempt deletions afterCloseis called. While the mutex protects against races, consider adding a context or shutdown signal to stop ongoing eviction goroutines when the cache is closed for cleaner resource management.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cache/memory.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cache/memory.go (1)
internal/cache/cache.go (1)
ErrCacheMiss(10-10)
🔇 Additional comments (3)
internal/cache/memory.go (3)
1-14: LGTM! Clean structure with appropriate concurrency primitives.The package imports and struct definition are well-designed. Using
sync.RWMutexis the right choice for a cache with read-heavy workloads.
23-34: LGTM! Correct concurrent read implementation.The
Getmethod properly uses read locking and returns the appropriate error for cache misses.
73-83: LGTM! Well-implemented testing helper.The
snapshotmethod correctly uses read locking and efficiently copies the cache state usingmaps.Copy. This is a clean testing utility.
Replacing Redis cache store with memory by default, but keeping an option to still use Redis if wanted.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.