Skip to content

refactor: remove global caches object#3167

Merged
miparnisari merged 2 commits into
mainfrom
server-owns-the-caches
Jun 9, 2026
Merged

refactor: remove global caches object#3167
miparnisari merged 2 commits into
mainfrom
server-owns-the-caches

Conversation

@miparnisari

@miparnisari miparnisari commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Solves #3156 (comment) by simplifying how our caching works.

Right now, each cache is registered in a global caches sync.Map, which means that if you try to spin up 2 spicedb servers within the same process (as our tests do) and both have caching turned on, a panic will occur. This PR fixes that.

Comment thread pkg/cache/metrics.go
)
)

var caches sync.Map

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this PR is to get rid of this global var.

@github-actions github-actions Bot added the area/cli Affects the command line label Jun 9, 2026
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.70588% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/cache/cache_otter.go 62.50% 11 Missing and 4 partials ⚠️
pkg/cache/standard.go 0.00% 2 Missing ⚠️
pkg/cmd/server/cacheconfig.go 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

github-actions[bot]

This comment was marked as off-topic.

@miparnisari miparnisari marked this pull request as ready for review June 9, 2026 17:04
@miparnisari miparnisari requested a review from a team as a code owner June 9, 2026 17:04
@miparnisari miparnisari enabled auto-merge (squash) June 9, 2026 17:09
@miparnisari miparnisari changed the title refactor: now, the server object owns the caches refactor: remove global caches object Jun 9, 2026
@github-actions github-actions Bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Jun 9, 2026
@miparnisari miparnisari force-pushed the server-owns-the-caches branch from 34e51cd to 3fbf9fe Compare June 9, 2026 17:28
Comment thread pkg/cache/cache_otter.go
// registration failure, already-registered metrics are rolled back.
func (wtc *otterCache[K, V]) registerMetrics(registerer prometheus.Registerer) error {
labels := prometheus.Labels{"cache": wtc.name}
collectors := []prometheus.Collector{

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i moved cache/metrics to here because i disliked the indirection. This does mean, though, that if we ever decide to add a new cache impl we will have to bring this too. I don't think we'll add a cache impl any time soon so that's why i did this

@tstirrat15 tstirrat15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread pkg/cache/cache_otter.go
Comment on lines +69 to +72
name: name,
cache: cache,
metrics: otterMetrics{atomic.Uint64{}, counter},
ttl: config.DefaultTTL,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@miparnisari miparnisari merged commit a8858d8 into main Jun 9, 2026
103 of 112 checks passed
@miparnisari miparnisari deleted the server-owns-the-caches branch June 9, 2026 19:02
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/cli Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) Skip-Changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants