[APIP] Add a resuable caching utility#2227
Conversation
DDH13
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummaryThis pull request adds a new generic, thread-safe in-memory cache utility for the SDK, introducing a common cache interface, an ChangesCache API primitives (
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description covers Purpose, Goals, Approach, and Key changes but lacks several required template sections including User stories, Documentation, Automation tests, Security checks, Samples, Related PRs, and Test environment. | Complete all required template sections: add User stories, Documentation links, detailed Unit/Integration test coverage, Security checks checklist, Samples details, Related PRs, and Test environment specifications. | |
| Docstring Coverage | Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title partially captures the main change by referencing a reusable caching utility, but contains a typo ('resuable' instead of 'reusable') and lacks specificity about the implementation details introduced. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 golangci-lint (2.12.2)
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies"
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 @coderabbitai help to get the list of available commands and usage tips.
![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)
sdk/core/utils/inmemorycache_test.go (1)
363-366: ⚡ Quick winUse clearer key generation for large iteration ranges.
The pattern
string(rune('0'+i))produces non-digit characters wheni >= 10(e.g., ':' for 10, ';' for 11). While functionally valid, consider using a more intuitive approach for better readability and debugging.♻️ Clearer alternative
// Trigger evictions by overfilling. for i := 0; i < 100; i++ { - _ = c.Set(ctx, CacheKey{Key: "evict" + string(rune('0'+i))}, "v") + _ = c.Set(ctx, CacheKey{Key: fmt.Sprintf("evict%d", i)}, "v") }Note: This requires importing
"fmt".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/core/utils/inmemorycache_test.go` around lines 363 - 366, The key generation pattern string(rune('0'+i)) produces non-digit characters when i >= 10 (e.g., ':' for i=10), which is confusing for debugging. Replace this pattern in the loop that generates cache keys with "evict" prefix by using fmt.Sprintf("evict%d", i) instead, which will produce clear, readable keys like "evict0", "evict1", ..., "evict99". This requires importing the "fmt" package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/core/utils/inmemorycache_test.go`:
- Around line 241-260: The test TestCleanupExpiredPreservesNonExpired claims to
verify that CleanupExpired preserves non-expired entries, but it only tests a
separate cache with zero TTL. To properly test the intended behavior, add
entries to cache c after the initial sleep (between the sleep and before calling
c.CleanupExpired) so that c contains both expired and non-expired entries, then
verify that GetStats().Size reflects only the expired entry being removed while
the new non-expired entries are preserved.
In `@sdk/core/utils/inmemorycache.go`:
- Around line 165-182: The eviction check in the Set method (checking if
len(c.cache) > c.size) happens after inserting the new entry, which under LFU
policy can cause the just-inserted entry to be evicted immediately. Move the
eviction logic to execute before creating the new inMemoryCacheEntry and pushing
to the accessOrder list, and add a capacity check to skip admission when
capacity is non-positive. This ensures that for LFU eviction policy, existing
entries are removed to make room before the new entry is added to the cache.
---
Nitpick comments:
In `@sdk/core/utils/inmemorycache_test.go`:
- Around line 363-366: The key generation pattern string(rune('0'+i)) produces
non-digit characters when i >= 10 (e.g., ':' for i=10), which is confusing for
debugging. Replace this pattern in the loop that generates cache keys with
"evict" prefix by using fmt.Sprintf("evict%d", i) instead, which will produce
clear, readable keys like "evict0", "evict1", ..., "evict99". This requires
importing the "fmt" package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffb122fe-0f21-48e9-b50d-6c72825e5cad
📒 Files selected for processing (3)
sdk/core/utils/cache.gosdk/core/utils/inmemorycache.gosdk/core/utils/inmemorycache_test.go
DDH13
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (4)
sdk/core/utils/cache/inmemorycache_test.go (3)
122-123: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert
Seterrors instead of discarding them.Line 122 and similar sites ignore
Seterrors (_ = c.Set(...)), which can let tests pass when setup actually failed. Preferrequire-style checks (orif err != nil { t.Fatalf(...) }) for all setup writes.Suggested pattern
- _ = c.Set(ctx, key, "first") + if err := c.Set(ctx, key, "first"); err != nil { + t.Fatalf("Set failed: %v", err) + }Also applies to: 142-142, 169-170, 192-192, 227-229, 247-248, 255-256, 283-285, 322-323, 345-347, 383-384, 453-453
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/core/utils/cache/inmemorycache_test.go` around lines 122 - 123, The test code ignores errors returned by c.Set() calls by assigning them to blank identifiers, which can mask setup failures and allow tests to pass despite initialization errors. Replace all instances where c.Set() errors are discarded with proper error assertions using require.NoError(t, err) or explicit error checks with t.Fatalf() to ensure setup operations succeed before proceeding with test assertions. This applies to all locations mentioned in the comment where c.Set() is called without error checking.
199-199: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace fixed sleeps with deadline-based polling to reduce test flakiness.
Lines 199, 234, and 250 use fixed
time.Sleep(...)assumptions. These can be flaky on slower CI workers. Prefer polling with a bounded timeout until the expected state is reached.Also applies to: 234-234, 250-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/core/utils/cache/inmemorycache_test.go` at line 199, The fixed time.Sleep calls at lines 199, 234, and 250 in the test create flakiness on slower CI workers. Replace each of these three fixed sleep statements with a deadline-based polling approach that repeatedly checks for the expected condition within a bounded timeout (e.g., using time.NewTimer or context.WithTimeout) instead of sleeping unconditionally for a fixed duration. This makes the tests more reliable and faster on all machines by allowing them to proceed as soon as the expected state is reached rather than waiting for an arbitrary sleep duration.
442-461: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd postcondition assertions to the concurrency test.
TestConcurrentAccesscurrently verifies completion only. Add basic invariants afterwg.Wait()(for example,Size <= MaxSize, cache still enabled, and non-negative stats) so regressions in concurrent behavior are observable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/core/utils/cache/inmemorycache_test.go` around lines 442 - 461, The TestConcurrentAccess function needs postcondition assertions added after the wg.Wait() call to verify cache invariants and detect concurrent behavior regressions. After all goroutines complete, add checks to verify that the cache size does not exceed the configured MaxSize, the cache remains in an enabled state, and all statistics values returned from GetStats() are non-negative. Use standard testing assertions (such as t.Errorf or t.Fatalf) to fail the test if any of these invariants are violated, ensuring the concurrent access does not corrupt the cache state or statistics.sdk/core/utils/cache/inmemorycache.go (1)
229-237: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor:
time.Now()called twice in Get pathConsider reusing the
nowvalue from line 229's expiry check at line 237 forlastAccessassignment. This avoids a redundant syscall on the hot path.Suggested change
- if !entry.ExpiryTime.IsZero() && time.Now().After(entry.ExpiryTime) { + now := time.Now() + if !entry.ExpiryTime.IsZero() && now.After(entry.ExpiryTime) { c.deleteEntry(key, entry) c.missCount.Add(1) c.logger.Debug("cache miss: entry expired", slog.String("key", key.Key)) var zero T return zero, false } - entry.lastAccess = time.Now() + entry.lastAccess = now🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/core/utils/cache/inmemorycache.go` around lines 229 - 237, The Get method is calling time.Now() twice on the hot path which results in redundant syscalls. Store the result of time.Now() in a local variable (e.g., `now`) before the expiry check at line 229, then reuse that same `now` variable both for the entry.ExpiryTime comparison and for the entry.lastAccess assignment at line 237, eliminating the second syscall.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sdk/core/utils/cache/inmemorycache_test.go`:
- Around line 122-123: The test code ignores errors returned by c.Set() calls by
assigning them to blank identifiers, which can mask setup failures and allow
tests to pass despite initialization errors. Replace all instances where c.Set()
errors are discarded with proper error assertions using require.NoError(t, err)
or explicit error checks with t.Fatalf() to ensure setup operations succeed
before proceeding with test assertions. This applies to all locations mentioned
in the comment where c.Set() is called without error checking.
- Line 199: The fixed time.Sleep calls at lines 199, 234, and 250 in the test
create flakiness on slower CI workers. Replace each of these three fixed sleep
statements with a deadline-based polling approach that repeatedly checks for the
expected condition within a bounded timeout (e.g., using time.NewTimer or
context.WithTimeout) instead of sleeping unconditionally for a fixed duration.
This makes the tests more reliable and faster on all machines by allowing them
to proceed as soon as the expected state is reached rather than waiting for an
arbitrary sleep duration.
- Around line 442-461: The TestConcurrentAccess function needs postcondition
assertions added after the wg.Wait() call to verify cache invariants and detect
concurrent behavior regressions. After all goroutines complete, add checks to
verify that the cache size does not exceed the configured MaxSize, the cache
remains in an enabled state, and all statistics values returned from GetStats()
are non-negative. Use standard testing assertions (such as t.Errorf or t.Fatalf)
to fail the test if any of these invariants are violated, ensuring the
concurrent access does not corrupt the cache state or statistics.
In `@sdk/core/utils/cache/inmemorycache.go`:
- Around line 229-237: The Get method is calling time.Now() twice on the hot
path which results in redundant syscalls. Store the result of time.Now() in a
local variable (e.g., `now`) before the expiry check at line 229, then reuse
that same `now` variable both for the entry.ExpiryTime comparison and for the
entry.lastAccess assignment at line 237, eliminating the second syscall.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e751b880-69f2-4bb9-abb4-b33d9c82e01d
📒 Files selected for processing (3)
sdk/core/utils/cache/cache.gosdk/core/utils/cache/inmemorycache.gosdk/core/utils/cache/inmemorycache_test.go
…handling in InMemoryCache
2 participants
Related to #2131
This pull request introduces a new generic, thread-safe in-memory cache utility to the SDK, supporting both LRU (Least Recently Used) and LFU (Least Frequently Used) eviction policies. It provides a flexible interface for cache operations, detailed cache statistics, and expiration handling. The implementation is designed to be reusable for different types of values and to be easily integrated into SDK policy logic.
Key changes:
New cache interface and types:
Cacheinterface incache.godefining standard cache operations, aCacheKeytype for keys, aCacheEntrystruct for values with expiration, and aCacheStatstruct for statistics.In-memory cache implementation:
InMemoryCacheininmemorycache.go, supporting LRU and LFU eviction, thread safety, generic value types, TTL-based expiration, and cache statistics tracking.NewInMemoryCacheconstructor to configure cache size, TTL, and eviction policy.Eviction and expiration logic:
Thread safety and statistics: