snapshot query prototype#186
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
reeceyang
force-pushed
the
reece/snapshot-query-prototype
branch
from
0e703d1 to
4296141
Compare
| console.debug(`[main] generation ${generation} behind: ${delayMs}ms`); | ||
|
|
||
| // Read snapshot of pendingStart and pendingCompletions | ||
| const { pending, completed } = await ctx.runQuery( |
There was a problem hiding this comment.
this call would be a snapshot query
ianmacartney
left a comment
ianmacartney
left a comment
There was a problem hiding this comment.
A couple early thoughts:
- If we don't find anything to do in updateRunStatus, we still need to fetch once without it being a snapshot query - so we transactionally change from running to idle and there isn't a race where a completing task doesn't start the loop but the loop misses the completed task
- We could stop using segment numbers altogether - though that's a bit orthogonal
- We'd likely want a version of it that we run from updateRunStatus that checks if there's anything to do... but then again part of the point of that function was to break up the transaction. so maybe we could just have loop:main schedule itself and cut that second one out entirely - a lot of it is just carefully fetching a little more data to avoid OCC's. Doing a full check in a snapshot query is strictly better I think...
- We could do the recovery & reporting from a snapshot to avoid OCC's when trying to check the status of things that are changing (e.g. reading .count on tables!)
| .withIndex("segment", (q) => | ||
| q | ||
| .gte("segment", incomingSegmentCursor - CURSOR_BUFFER_SEGMENTS) | ||
| .lte("segment", segment), |
There was a problem hiding this comment.
I don't think we need to limit the upper bound anymore!
| .query("pendingStart") | ||
| .withIndex("segment", (q) => | ||
| q | ||
| .gte("segment", incomingSegmentCursor - CURSOR_BUFFER_SEGMENTS) |
There was a problem hiding this comment.
once we get index caching, we can drop this!
|
|
||
| export const getPending = internalQuery({ | ||
| args: { | ||
| toSchedule: v.number(), |
There was a problem hiding this comment.
if we could drop this from the args, we could get query caching when nothing new has been added to pendingStart, at the cost of over-fetching work to do.. maybe that's over-kill. Maybe we could round / etc. But that's an over-optimization, and generally we wouldn't expect to get cache hits since we're presumably immediately removing things from it
| // Read pendingCancelation, deleting from pendingStart. If it's still running, queue to cancel. | ||
| console.time("[main] pendingCancelation"); | ||
| await handleCancelation(ctx, state, segment, console, toCancel); | ||
| const canceledWorkIds = await handleCancelation( |
There was a problem hiding this comment.
why not read from cancelation at the same time as pending start/completed? we could even check when fetching if there were any overlap between canceled & pending start - though that isn't sufficient to know it's not in the table at all
| console.time("[main] pendingStart"); | ||
| await handleStart(ctx, state, segment, console, globals); | ||
| const filteredPending = pending.filter( | ||
| (p) => !canceledWorkIds.has(p.workId), |
There was a problem hiding this comment.
note that the work could be in the canceled table but not fetched in the latest batch of canceled ids
2 participants
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.