Skip to content

fix(elasticache): dedupe missing engines and redis valkey family - #158

Merged
cristim merged 2 commits into
mainfrom
fix/150-elasticache-engine-dedupe
Sep 29, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/150-elasticache-engine-dedupe

Conversation

@cristim

@cristim cristim commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Summary

Recent ElastiCache reservations now suppress matching Redis and Valkey recommendations even when AWS omits the reservation engine. Unknown engines match conservatively and emit a warning. Known Memcached reservations remain separate.

Root cause and fix

The duplicate checker used exact engine keys, so an empty engine or Redis reservation could never match a Valkey recommendation. AWS ElastiCache now has dedupe-only engine keys, scoped to both supported service aliases. Exact family counts are consumed before unknown-engine counts, with each count deducted once. Recommendation engines and purchase offering parameters remain unchanged.

The MemoryDB regression now obtains its recommendation through the real CE parser instead of hardcoding the parser's expected engine.

Regression proof

New count-budget and parser-to-SDK-client tests fail on the original production code at the expected suppression assertions, then pass with this fix. Cases cover missing engines, Redis/Valkey case variants, Memcached separation, partial counts, repeated rows, same-key wildcard matching, and mixed provider/service collections in both orders. A mutation of only the MemoryDB parser engine makes its relocated regression fail. An independent reviewer reproduced these failures and restored the changes before passing the suites.

Verification

  • Go 1.26.6: full workspace pkg/AWS race tests, build and vet passed. Final affected suites passed uncached race tests.
  • Standalone pkg tests passed; both modules' GOWORK=off go mod tidy -diff checks were empty.
  • golangci-lint 2.10.1: affected pkg/AWS packages passed with zero issues.
  • All applicable commit hooks passed without bypass.
  • Independent gpt-6-astra review and fresh committed-source tests approved exact integrated SHA 9b5ca4d61d2afe05ffca9109244dc225820297ef. This session explicitly uses that reviewer because the pinned Opus model is unavailable. CI remains a merge gate.

Evidence uses real library paths with injected SDK fixtures, not live cloud calls. Standalone AWS compilation remains blocked by the inherited published-pkg PurchaseResult.Cost mismatch tracked in #154. Workspace module resolution was verified; no dependency overrides were added.

Main a29ea574e5223cc05ec1cbef28865f5bc1a2496a, including #155, #156 and #157, was integrated with a normal merge that preserves published history. The resolution retains both the GCP dispatch and ElastiCache test groups, plus the MemoryDB recfilter import required by #156. Combined uncached race tests passed across shared recfilter, AWS recommendations/ElastiCache/MemoryDB and GCP computeengine; build, vet, pinned lint and applicable hooks passed. The independent reviewer re-read the committed resolution and passed named AWS/GCP/shared regressions in those five packages at the exact integrated SHA.

Labels mirror #150; the issue does not carry triaged.

Closes #150

Summary by CodeRabbit

  • Bug Fixes
    • Improved duplicate detection for AWS ElastiCache recommendations by matching Redis and Valkey reservations together while keeping Memcached separate.
    • Recommendations can now be reduced by matching specific-engine and wildcard reservations, without applying the same reservation capacity more than once.
    • Empty or unrecognized ElastiCache reservation engines can match recommendations for any cache engine. A warning is logged when a reservation has no engine information.
    • Improved filtering of AWS ElastiCache and MemoryDB recommendations that match existing reservations.

@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm effort/xs Trivial / one-liner type/bug Defect labels Sep 28, 2026
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: 5a821ec7-03d1-4b98-9075-80539910fd19

📥 Commits

Reviewing files that changed from the base of the PR and between a29ea57 and 9b5ca4d.

📒 Files selected for processing (6)
  • pkg/recfilter/dedupe.go
  • pkg/recfilter/dedupe_test.go
  • providers/aws/recommendations/parser_services_test.go
  • providers/aws/services/elasticache/client.go
  • providers/aws/services/elasticache/client_test.go
  • providers/aws/services/memorydb/client_test.go
💤 Files with no reviewable changes (1)
  • providers/aws/services/memorydb/client_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


📝 Walkthrough

Walkthrough

AWS ElastiCache deduplication now matches Redis and Valkey recommendations under one engine identity. Empty reservation engines act as wildcards. Matching commitments are consumed against recommendation counts, and AWS reservation and parser tests cover these cases.

Changes

AWS commitment deduplication

Layer / File(s) Summary
Engine matching and commitment allocation
pkg/recfilter/dedupe.go, pkg/recfilter/dedupe_test.go
ElastiCache keys map Redis and Valkey to the same identity and map empty engines to a wildcard. Matching counts are consumed from exact matches before wildcard matches; partial coverage reduces the recommendation count. Tests cover matching scope and count allocation.
AWS reservation handling and parser coverage
providers/aws/services/elasticache/client.go, providers/aws/services/elasticache/client_test.go, providers/aws/recommendations/parser_services_test.go, providers/aws/services/memorydb/client_test.go
ElastiCache reservations with empty engines remain included and produce a warning. Parser tests cover ElastiCache and MemoryDB reservation deduplication. The MemoryDB client deduplication regression test is removed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 9b5ca

No actionable issue is established; the change is ready for normal merge checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR satisfies the coding requirements in [#150]. GetExistingCommitments logs a warning for an empty ProductDescription and keeps the empty engine. dedupeEngine maps empty ElastiCache engines …
Out of Scope Changes check ✅ Passed The changed implementation and tests support [#150]. The deduplication changes, warning test, SDK reservation stubs, ElastiCache parser tests, and MemoryDB parser test all validate the requested behav…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: ElastiCache deduplication for missing engines and Redis/Valkey family equivalence.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@cristim
cristim merged commit b3b4cb5 into main Sep 29, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner priority/p2 Backlog-worthy severity/medium Moderate harm type/bug Defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(elasticache): empty engine fails dedupe open; Redis OSS RIs covering Valkey recs aren't deduped

1 participant