feat: add caching mechanism to GetReleaseTargetsForDeploymentAndEnvir…#851
Merged
adityachoudhari26 merged 2 commits intomainfrom Mar 19, 2026
Merged
feat: add caching mechanism to GetReleaseTargetsForDeploymentAndEnvir…#851adityachoudhari26 merged 2 commits intomainfrom
adityachoudhari26 merged 2 commits intomainfrom
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request adds a new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Store as Store (Query Method)
participant Cache as gocache.Cache
participant DB as PostgreSQL
Caller->>Store: GetReleaseTargets(ctx, id)
Store->>Cache: Get(cacheKey)
alt Cache Hit
Cache-->>Store: cached targets
Store-->>Caller: return targets
else Cache Miss
Store->>Store: Parse id(s) as UUID(s)
Store->>DB: GetQueries().GetReleaseTargets...(ctx, uuid)
DB-->>Store: []Row
Store->>Store: Convert rows -> []oapi.ReleaseTarget
Store->>Cache: SetDefault(cacheKey, targets)
Store-->>Caller: return targets
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/store/releasetargets/get_for_workspace.go (1)
58-60: Consider memory implications of caching workspace-level results.Workspaces may contain a large number of release targets. Caching entire
[]oapi.ReleaseTargetslices per workspace could lead to significant memory usage if many workspaces are queried.If this is a concern, consider adding a size threshold or LRU eviction policy. Otherwise, if workspaces are expected to have bounded target counts, this is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/store/releasetargets/get_for_workspace.go` around lines 58 - 60, The current caching of entire workspace slices via s.cache.SetDefault(workspaceID, targets) may blow memory for large workspaces; modify the code in GetForWorkspace to only cache when the number of targets is below a threshold (e.g., add const maxCachedTargets = N and if len(targets) <= maxCachedTargets then s.cache.SetDefault(...)), or replace/augment s.cache with an LRU-capable cache and use that instead so eviction controls memory; refer to s.cache, workspaceID and targets when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/store/releasetargets/release_target_exists.go`:
- Around line 63-65: The current logic caches both true and false results via
s.cache.SetDefault(cacheKey, exists), which can serve stale false negatives;
update the code to only cache positive confirmations (i.e., call
s.cache.SetDefault(cacheKey, exists) only when exists == true) or make caching
behavior configurable (e.g., a short TTL or a flag) so that false results are
not cached long-term; modify the code around s.cache, cacheKey, and the variable
exists to implement this change.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/store/releasetargets/get_for_workspace.go`:
- Around line 58-60: The current caching of entire workspace slices via
s.cache.SetDefault(workspaceID, targets) may blow memory for large workspaces;
modify the code in GetForWorkspace to only cache when the number of targets is
below a threshold (e.g., add const maxCachedTargets = N and if len(targets) <=
maxCachedTargets then s.cache.SetDefault(...)), or replace/augment s.cache with
an LRU-capable cache and use that instead so eviction controls memory; refer to
s.cache, workspaceID and targets when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8eefe409-889c-48ac-bc76-8204aa94f008
📒 Files selected for processing (6)
apps/workspace-engine/pkg/store/releasetargets/get_for_deployment.goapps/workspace-engine/pkg/store/releasetargets/get_for_deployment_environment.goapps/workspace-engine/pkg/store/releasetargets/get_for_resource.goapps/workspace-engine/pkg/store/releasetargets/get_for_workspace.goapps/workspace-engine/pkg/store/releasetargets/options.goapps/workspace-engine/pkg/store/releasetargets/release_target_exists.go
Comment on lines
+63
to
+65
| if s.cache != nil { | ||
| s.cache.SetDefault(cacheKey, exists) | ||
| } |
Contributor
There was a problem hiding this comment.
Caching false results may cause staleness issues.
Caching a false existence result means that if a release target is created shortly after, the cached false will be served until TTL expiry. This could cause callers to incorrectly believe the target doesn't exist.
Consider whether caching only true results (existence confirmed) is safer for your use case, or if the TTL is short enough that temporary staleness is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/store/releasetargets/release_target_exists.go`
around lines 63 - 65, The current logic caches both true and false results via
s.cache.SetDefault(cacheKey, exists), which can serve stale false negatives;
update the code to only cache positive confirmations (i.e., call
s.cache.SetDefault(cacheKey, exists) only when exists == true) or make caching
behavior configurable (e.g., a short TTL or a flag) so that false results are
not cached long-term; modify the code around s.cache, cacheKey, and the variable
exists to implement this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…onment
Summary by CodeRabbit