-
Notifications
You must be signed in to change notification settings - Fork 12
auto-ttl: Add galaxy-level TTL config #76
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
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.
One minor nit...
galaxycache.go
Outdated
| } | ||
|
|
||
| // WithGetTTL allows the client to specify a default TTL for the galaxy, with optional jitter | ||
| // jitter may be 0 to always set the TTL to exactly ttl in the future after calling the BackendGetter. |
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.
This sentence seems to be worded strangely... (Same on L547)
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.
Following our in-person discussion, I've rewritten the semantics to only jitter to earlier-times, and rewritten/expanded these doc-comments. (and updated the package-level doc and a mention in the top-level README -- that needs a more general update that I'll try to get to after the current project is done)
I also moved WithPeekTTL into the PeekPeerCfg struct.
7a88c7e to
0a10226
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.
Couple little nits.
galaxycache.go
Outdated
| // | ||
| // Expiration times of values with an expiration earlier than maxTTL in the | ||
| // future are left alone no matter the source. (e.g. the value may have | ||
| // originally had a TTL in the same range, but have been sitting in the cache |
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.
I'm not sure I'm following here...
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.
I've done a little rewording. Hopefully that helps?
galaxycache.go
Outdated
| // When used, Jitter values should be large enough that over a reasonable | ||
| // number of maxTTL intervals, keys that are continually accessed will be | ||
| // spread their expiration across the entire interval. |
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.
| // When used, Jitter values should be large enough that over a reasonable | |
| // number of maxTTL intervals, keys that are continually accessed will be | |
| // spread their expiration across the entire interval. | |
| // When used, Jitter values should be large enough that, over a reasonable | |
| // number of maxTTL intervals, keys that are continually accessed will | |
| // spread their expiration across the entire interval. |
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 fixes.
applied.
galaxycache.go
Outdated
| // Negative TTLs and jitter values are silently ignored, and jitter values that | ||
| // are greater than maxTTL will be capped at maxTTL. | ||
| PeekedValueMaxTTL time.Duration `dialsdesc:"max time in the future to allow a value pulled in via a Peek request have their expiration"` | ||
| PeekedValueTTLJitter time.Duration `dialsdesc:"max time to reduce 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.
nit: truncated dialsdesc
galaxycache.go
Outdated
| // When used, Jitter values should be large enough that over a reasonable | ||
| // number of maxTTL intervals, keys that are continually accessed will be | ||
| // spread their expiration across the entire interval. |
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.
| // When used, Jitter values should be large enough that over a reasonable | |
| // number of maxTTL intervals, keys that are continually accessed will be | |
| // spread their expiration across the entire interval. | |
| // When used, Jitter values should be large enough that, over a reasonable | |
| // number of maxTTL intervals, keys that are continually accessed will | |
| // spread their expiration across the entire interval. |
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.
0a10226 to
3ede1a3
Compare
Directly support jittering the TTL/expiry to make it easy to do the right thing. (bound the expiration as necessary)
3ede1a3 to
bf29e3b
Compare
Directly support jittering the TTL/expiry to make it easy to do the
right thing. (bound the expiration as necessary)