-
Notifications
You must be signed in to change notification settings - Fork 12
TTL/Expiration setting & propagation #75
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
Conversation
go 1.25 is 3 months old and the 1.26 freeze is about to begin. Bump go.mod to 1.24 so we can use weak pointers without having to add new build tags.
83dfd1e to
58ccee9
Compare
Leverage the container/heap stdlib package to manage a slice-backed heap for a first-pass.
58ccee9 to
708c389
Compare
ec7a68d to
91a715c
Compare
sergiosalvatore
left a comment
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.
Few small things...
galaxycache.go
Outdated
| return BackendGetInfo{}, l.be.Get(ctx, key, dest) | ||
| } | ||
|
|
||
| // BackendGetterWithInfo provides the |
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.
Looks like a truncated docblock.
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.
Indeed it does.
Apparently, I also forgot to write the rest of the doc-comment on BackendGetInfo, too.
galaxycache.go
Outdated
| // The returned [BackendGetter] tries (but does not guarantee) to run only one | ||
| // [Galaxy.Get] is called once for a given key across an entire set of peer | ||
| // processes. Concurrent callers both in the local process and in | ||
| // other processes receive copies of the answer once the original Get | ||
| // completes. |
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.
| // The returned [BackendGetter] tries (but does not guarantee) to run only one | |
| // [Galaxy.Get] is called once for a given key across an entire set of peer | |
| // processes. Concurrent callers both in the local process and in | |
| // other processes receive copies of the answer once the original Get | |
| // completes. | |
| // The returned [BackendGetter] tries (but does not guarantee) to run only one | |
| // instance of [Galaxy.Get] for a given key across an entire set of peer | |
| // processes. Concurrent callers both in the local process and in | |
| // other processes receive copies of the answer once the original Get | |
| // completes. |
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.
wow, that wording was rather messed up.
It was a copy of NewGalaxy's doc-comment with a couple minor adjustments.
I've substantially reworded it. (your suggestion didn't seem sufficient)
galaxycache.go
Outdated
| } | ||
|
|
||
| type GetInfo struct { | ||
| // TODO: include information about hit-level, backend fetches and any TTL (when implemented). |
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.
Does this comment still apply?
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.
Good point.
I've removed TTL from the comment, moved it below Expiry and added some more doc-comments.
jsoncodec/backend_getter.go
Outdated
| // backendGetter is an adapter type that implements galaxycache.BackendGetter | ||
| type backendGetterWithInfo[T any] func(ctx context.Context, key string) (T, galaxycache.BackendGetInfo, error) | ||
|
|
||
| // Get populates dest with the value identified by key |
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 populates dest with the value identified by key | |
| // Get populates dest with the value identified by `key`. |
I might also backtick-quote the other instances of key so it's extra clear that we're referring to the argument.
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.
Good call!
Done. (same in protocodec)
| xt.updateLocation(len(e.ents) - 1) | ||
| } | ||
|
|
||
| const sentinelOffset = math.MaxInt |
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.
Should this be math.MaxUInt given that offset is a uint?
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.
Good question.
Unfortunately, we convert it to an int, so the value would be harder to compare after the conversion if we used math.MaxUint.
I've added a doc-comment.
| } | ||
|
|
||
| // Add adds a value to the cache. | ||
| func (c *TypedCache[K, V]) Add(key K, value V) { |
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.
Should this just call AddExpiring with a zeroed time.Time{}?
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.
oh yeah, it can do that now.
My original implementation had split them over separate files with build tags so Add was available for all go versions, but I decided to just bump the go.mod to go 1.24, so I could merge it all back into one file.
Done.
Add expiry support for go 1.24+ with the help of weak pointers. Care has to be taken to make sure that Get doesn't return an expired value (and Add..., Remove... and friends are also configured for removing any expired entries before removing anything from the LRU stack).
Make it possible to remove entries that have been evicted from the cache by returning an opaque handle that can be passed to a Remove method.
Remove entries from the expiry tracker when they get removed.
Add a new BackendGetterWithInfo interface that allows a BackendGetter implementation to return an expiration time as well as functioning with the normal Codec mechanism for extracting the values. Plumb the BackendGetInfo with a new method on Universe, additional return values on Fetch & Peek, and update tests+http&grpc impls. Follow-up commits will update the grpc and http methods to propagate expirations.
Add expiry fields to both the Get and Peek responses so we can propagate expiration timestamps across process-boundaries.
Use a custom header format that includes nanoseconds so we don't lose fidelity. (note: we don't need to use the same format when parsing, and can instead use the stock http.ParseTime on the receiving side because Go's time.Parse automatically includes fractional seconds.)
Replace a deprecated call.
91a715c to
fcffc54
Compare
The tests on PR#318 flaked because of the log-after-test-completion race-detection. Avoid logging if the context is canceled. (this won't 100% eliminate the race, but it'll bring it down to a super-rare race)
Upgrade genproto within the compattest/peerv1.2 submodule to actually fix it, too.
fcffc54 to
3163a8c
Compare
sergiosalvatore
left a comment
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.
A couple more little nits...
| e.ents[i], e.ents[j] = e.ents[j], e.ents[i] | ||
| } | ||
|
|
||
| func (e *expiryHeap[T]) Push(x any) { |
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.
nit: we probably want a trivial doc comment...
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.
Added.
Note that expiryHeap is unexported.
| e.ents = append(e.ents, xt) | ||
| } | ||
|
|
||
| func (e *expiryHeap[T]) Pop() any { |
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.
nit: doc comment for exported method.
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.
added.
(same for EntryHandle)
galaxycache.go
Outdated
| // TODO: include information about hit-level, backend fetches and any TTL (when implemented). | ||
| // If non-zero, Expiry provides an expiration time after which | ||
| // Galaxycache should not return this value (and should ideally evict | ||
| // it from the cache to prevent unexpired, but more recently-outched |
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.
nit:
s/outched/touched/
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
peers.go
Outdated
|
|
||
| // RemoteFetcherWithInfo is an extension of [RemoteFetcher], allowing | ||
| // [RemoteFetcher] implementations to optionally provide [BackendGetInfo]. (the | ||
| // zero-value is assumeed otherwise) |
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.
nit:
s/assumeed/assumed/
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
Add support for expiration-tracking within the
lruimplementation, and hook that up through theGalaxytype.Add a new
GalaxyWithBackendInfomethod toUniverseto allow users to provide hydration callbacks that returnBackendGetInfothat includes an optional expiration. (the zero-valuedtime.Timeis considered "no expiration").The
lrusupport leverages a heap to track the next expiration. It evicts all expired entries before evicting anything.