-
Notifications
You must be signed in to change notification settings - Fork 6
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
indexer: add persistence plugin #88
Conversation
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.
Looks good. It needs tests to make sure everything is working fine but I couldn't catch anything wrong with it.
|
||
describe("Sqlite Persistence", () => { | ||
let db: Database<sqlite3.Database, sqlite3.Statement>; | ||
let store: SqlitePersistence; |
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.
Should be called persistence :)
}); | ||
|
||
indexer.hooks.hook("connect:before", async ({ request }) => { | ||
const store = new SqlitePersistence(db); |
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 think it's possible to instantiate the persistence once and share the instance between the hooks (like you do with db
).
|
||
indexer.hooks.hook("sink:flush", async ({ endCursor }) => { | ||
const store = new SqlitePersistence(db); | ||
if (endCursor) store.put(endCursor); |
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 (endCursor) store.put(endCursor); | |
if (endCursor) { | |
store.put(endCursor); | |
} |
import { afterAll, beforeAll, describe, expect, it } from "vitest"; | ||
import { SqlitePersistence } from "./persistence"; | ||
|
||
describe("Sqlite Persistence", () => { |
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 issue I have with these tests is that they're very dependent on the order in which they're run.
I would change them to have only 2 cases:
Test 1: assert there's no data, then insert value, check that value was stored, update value, check that value was updated
Test 2: assert there's no data, insert value, check value was stored, delete value, check there's no data.
For each test, you should create a db/persistence instance specific to that test. That way if in the future we run tests in parallel or if vitest changes how they order tests, test won't break.
|
||
type SqliteArgs = ISqlite.Config; | ||
|
||
export function sqlitePersistence<TFilter, TBlock, TRet>(args: SqliteArgs) { |
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.
BTW now that we have a mock stream, we can test plugins end to end.
Add a tests where you setup a mock indexer with the sqlitePersistence
plugin, run it over a sequence of messages and then assert that the stored cursor is the last endCursor
in your stream.
id TEXT NOT NULL PRIMARY KEY, | ||
order_key INTEGER NOT NULL, | ||
unique_key TEXT, | ||
UNIQUE(order_key, unique_key) |
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 don't think UNIQUE
is needed. We can safely remove 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.
Not needed because we have id
in this table.
27d3bde
to
e56cb30
Compare
e56cb30
to
9c93e23
Compare
add persistence plugin to persist the indexer’s state between restarts.