-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a lookaside cache in distributed.go for small CAS values #6203
Conversation
) | ||
|
||
const ( | ||
// Each hinted handoff is a digest (~64 bytes), prefix, and peer | ||
// (40 bytes). So keeping around 100000 of these means an extra 10MB | ||
// per peer. | ||
maxHintedHandoffsPerPeer = 100000 | ||
|
||
// Max size of an entry that can be cached in the lookaside cache. | ||
maxLookasideEntrySize = 100_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth adding a flag for this for if we're flushing it way too fast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (and defaulted to 10K)
for _, r := range stillMissing { | ||
buf, ok := results[r.GetDigest()] | ||
if ok { | ||
c.addLookasideEntry(r, buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a max size check here, i think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, fixed.
c.lookasideMu.RLock() | ||
entry, ok := c.lookaside.Get(k) | ||
c.lookasideMu.RUnlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get
does a mutate of the LRU so this might need to be an exclusive lock? (maybe worth also adding a concurrency test since the race detector didn't trigger)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
<<<All credit to @vadimberezniker for originally suggesting this and @jdhollen for instrumenting things to the point where we think it's a good idea>>>
This CL introduces a configurable size LRU in distributed.go to cache response data at read time when it's read from any remote node. If that same data is re-requested, it can be served directly from the read-through cache and skip the work of hitting a remote peer.
Doesn't cache values over 100K, and doesn't hold on to any value longer than 15 minutes (configurable).