Add ADR about adopting FusionCache for all our caching needs #722
Conversation
|
Claude finished @justindbaur's task —— View job Review Complete ✓I've completed a comprehensive review of this ADR document. Here are my findings: Overall Assessment✓ Ready to merge - This ADR is well-structured, complete, and has successfully addressed all previous review feedback. What's Been AddressedAll feedback from previous reviews has been properly incorporated:
Style Guide Compliance✓ Excellent adherence to repository documentation standards:
Content QualityStrengths:
Structure Completeness✓ All required ADR sections present:
Technical Accuracy✓ Correctly explains:
Reference Quality✓ All 6 reference links properly formatted:
Minor ObservationsNo blocking issues, but a couple of optional enhancements for future consideration:
These are purely optional suggestions and don't impact the quality or completeness of the ADR. Comparison with Existing ADRsThis ADR maintains consistency with other ADRs in the repository (e.g., ADR-0024, ADR-0026) in terms of:
Final Recommendation👍 Approve and merge - This document successfully:
The ADR effectively documents why FusionCache was chosen for Bitwarden's caching needs and provides a practical roadmap for adoption across the server codebase. |
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Deploying contributing-docs with
|
| Latest commit: |
f55cd44
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e9473fb.contributing-docs.pages.dev |
| Branch Preview URL: | https://fusion-cache-adr.contributing-docs.pages.dev |
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Bunch of little ⛏️s but reads well and explains the cache rationale.
|
|
||
| - New features desiring cache should use `IFusionCache` | ||
| - Finish up Caching package in server SDK (Target: Q1 2026) | ||
| - Individual migration plans for existing cache uses |
There was a problem hiding this comment.
🤔 What do we think about a Claude Skill created to help create a migration plan for each scenario it finds in our code base? Maybe the skill could even create the Jira story with suggested code change 🤯
|
|
||
| - Could make caching too easy to use when caching isn't the right solution all the time | ||
|
|
||
| ### Implementation plan |
There was a problem hiding this comment.
💭 We mentioned telemetry/monitoring our cache as a motivating factor for the project but we do not mention it as part of our implementation plan. I think that if it is important enough to be mentioned in the problem statement then it would also be found as part of the implementation plan.
Co-authored-by: Matt Bishop <matt@withinfocus.com>
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Just to be in sync, the latest Claude comment pretty much sums up what's still desired -- the high and medium priority items.
|
@withinfocus Should be all set other than the status one, should we roll this with Accepted already or leave it as Proposed? |
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Couple small things from the last commit, and yeah let's move this to Accepted as we have already discussed these options.
| ### Plan | ||
|
|
||
| - New features desiring cache should use `IFusionCache` | ||
| - Finish up Caching package in server SDK (Target: Q1 2026) |
There was a problem hiding this comment.
🎨 Let's remove any date commitments from this sort of document.
| - If only `IDistributedCache` is needed memory cache and backplane can be turned off | ||
| - If no `IDistributedCache` is needed it can be turned off and only memory and the backplane will | ||
| be used. | ||
| - Configure and document how to view caches hits and misses for your fusion cache uses |
There was a problem hiding this comment.
| - Configure and document how to view caches hits and misses for your fusion cache uses | |
| - Configure and document how to view caches hits and misses for your `IFusionCache` usages |
| @@ -0,0 +1,177 @@ | |||
| --- | |||
| adr: "0028" | |||
| status: Proposed | |||
There was a problem hiding this comment.
| status: Proposed | |
| status: Accepted |
Co-authored-by: Matt Bishop <matt@withinfocus.com>
🎟️ Tracking
📔 Objective
Adds a ADR about adopting
FusionCachefor all our caching needs, this is a little retrospective as we've already started down the road of using it in server but this documents the reasons it was chose and why it was chose over alternatives.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes