-
Notifications
You must be signed in to change notification settings - Fork 606
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
Moved indexedAt time to record instead of ipld-block #501
Conversation
indexedAt: db | ||
.selectFrom('ipld_block') | ||
.whereRef('ipld_block.cid', '=', ref('record.cid')) | ||
.select('indexedAt'), |
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.
If there's a missing block for a record, I think this could fail when trying to set indexedAt
to null
. It shouldn't really happen, but there's nothing at the db level preventing it either. Not sure if it's really necessary, but I think the fix would be to do something like this:
const indexedAtForRecordQb = db
.selectFrom('ipld_block')
.whereRef('ipld_block.cid', '=', ref('record.cid'))
.select('indexedAt')
await db
.updateTable('record')
.set({ indexedAt: indexedAtForRecordQb })
.whereExists(indexedAtForRecordQb)
.execute()
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.
ah nice. great point 👌
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.
Lookin good!
This migration moves the
indexedAt
time onipld_block
to the relevantrecord
This makes the migration in #500 much simpler (no need to track indexedAt time) & is also fixes a leaky abstraction -
ipld_block
is a repo storage concept &record
is an indexing layer concept.