Postgres optimized revision caching #555
Conversation
github-actions
bot
added
area/datastore
| updateGroup singleflight.Group | ||
| } | ||
|
|
||
| type validRevision struct { |
There was a problem hiding this comment.
👍 this is a small change but I like it
| @@ -23,7 +24,8 @@ const ( | |||
| SELECT COALESCE( | |||
| (SELECT MIN(%[1]s) FROM %[2]s WHERE %[3]s >= TO_TIMESTAMP(FLOOR(EXTRACT(EPOCH FROM NOW() AT TIME ZONE 'utc') * 1000000000 / %[4]d) * %[4]d / 1000000000)), | |||
| (SELECT MAX(%[1]s) FROM %[2]s) | |||
| );` | |||
| ), | |||
| %[4]d - CAST(EXTRACT(EPOCH FROM NOW() AT TIME ZONE 'utc') * 1000000000 as bigint) %% %[4]d;` | |||
There was a problem hiding this comment.
update the comment to note that it's also returning validForNanos now?
jakedt
force-pushed
the
postgres-revision-caching
branch
from
10185ed
to
0c7dc2e
Compare
|
Updated |
jakedt
force-pushed
the
postgres-revision-caching
branch
from
0c7dc2e
to
59b5901
Compare
| // it will remain valid. | ||
| type OptimizedRevisionFunction func(context.Context) (rev datastore.Revision, validFor time.Duration, err error) | ||
|
|
||
| // NewRemoteClockRevisions returns a RemoteClockRevisions for the given configuration |
| lastQuantizedRevision, err, _ := cor.updateGroup.Do("", func() (interface{}, error) { | ||
| log.Debug().Time("now", localNow).Time("valid", lastRevision.validThrough).Msg("computing new revision") | ||
|
|
||
| optimized, validFor, err := cor.optimizedFunc(ctx) |
There was a problem hiding this comment.
Add a panic here if it is not set? its slightly nicer than just an NPE
There was a problem hiding this comment.
At the cost of requiring an extra conditional every time.
There was a problem hiding this comment.
Yes, but I tend to think having panics when its an expected state makes sense; it would mean they forgot to call the assignment method, and that's certainly an expected bug
There was a problem hiding this comment.
I wish we had debug-only checks
There was a problem hiding this comment.
I've seen this done with build tags in https://github.com/dlsniper/debugger and in some etcd fault injection library that I can't find now. I assume the compiler removes no-op functions but I haven't checked.
|
|
||
| rvt := localNow. | ||
| Add(validFor). | ||
| Add(cor.maxRevisionStaleness) |
There was a problem hiding this comment.
Won't adding these push it above the max staleness?
There was a problem hiding this comment.
My definition for staleness is "time after which the new revision would have been selected". So in this case we allow for it to be used slightly longer than it's strictly valid for, but which gives us a minimum valid lifetime for any revision we ask the database to compute. E.g you can't get back a revision that's only good for 1 nanosecond.
There was a problem hiding this comment.
But why not just do max(validFor, maxRevisionStaleness) then?
| } | ||
|
|
||
| // CachedOptimizedRevisions does caching and deduplication for requests for optimized revisions. | ||
| type CachedOptimizedRevisions struct { |
There was a problem hiding this comment.
Move above the function defs for this type?
There was a problem hiding this comment.
I don't know. All of the fields are private, it's probably less important than the API to interact with it.
There was a problem hiding this comment.
True, but it was highly unexpected when I landed on the file
jakedt
force-pushed
the
postgres-revision-caching
branch
2 times, most recently
from
244d25c
to
6116688
Compare
jakedt
force-pushed
the
postgres-revision-caching
branch
from
6116688
to
3fb483c
Compare
No description provided.