-
Notifications
You must be signed in to change notification settings - Fork 308
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
Optimize database reindex #4558
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
Quality Gate passedIssues Measures |
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.
Overall looks good, nice perf wins 🎉
See questions on forAllResources
and transactions
|
||
const lastUpdated = resource.meta?.lastUpdated as string; | ||
currentTimestamp = lastUpdated; | ||
await this.withTransaction(async (conn) => { |
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.
Is this transaction necessary? Trying to imagine what it's needed for, versus letting the callback handle it.
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.
Hm, I see, you're using this to avoid per-resource transactions in the reindex step.
imho, that kinda breaks the abstraction of forAllResources
- I'm not sure the transaction is fully necessary in
reindexResource
- If it is both necessary and if this is a major performance win, then i think we should probably modify the API contract of
forAllResources
(maybe a "page" callback and a "resource" callback?)
In testing on localhost over resource types with 15k – 100k resources in the table, this combination of transaction batching and concurrent handling of the lookup tables reduced the overall full reindex time by 30–50% (saving on the order of several minutes for my small data set).
This PR also moves the reindex logic into an async worker that processes a batch of rows and then enqueues another job to handle the next chunk. This makes the job much more robust, and allows it to more safely run for extended periods of time.