rolemembershipcache: extract cache to new package #136514
Conversation
The caching logic was getting to be quite large, and it already was nicely contained in an easy to move struct. This is a purely mechanical refactor. Release note: None
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @fqazi and @rafiss)
pkg/sql/rolemembershipcache/cache.go line 0 at r1 (raw file):
It seems like none of the unit tests were self-contained enough to be copied over to the new package, is that right?
These tables all have fixed IDs, so looking them up by name is not necessary. Release note: None
rafiss
force-pushed
the
refactor-membership-cache
branch
from
b659da6 to
97edf1f
Compare
There was a problem hiding this comment.
tftr!
bors r+
Reviewable status:
pkg/sql/rolemembershipcache/cache.go line at r1 (raw file):
Previously, spilchen wrote…
It seems like none of the unit tests were self-contained enough to be copied over to the new package, is that right?
that's right, and one of the benefits of this refactor is that it makes that quite clear. i didn't want to make the new tests block the improvement to organizing the code.
though one related test is in pkg/sql/cacheutil. we started to make a general purpose package for caches that work like this one, but unfortunately haven't updated them all to use that package. each cache has subtle differences in implementation that makes the consolidation a bit tricky to get right.
cockroach/pkg/sql/cacheutil/cache_test.go
Line 21 in 9f61a38
The caching logic was getting to be quite large, and it already was
nicely contained in an easy to move struct.
This is a purely mechanical refactor.
sql, sessioninit: lookup tables by ID, not name
These tables all have fixed IDs, so looking them up by name is not
necessary.
Epic: None
Release note: None