-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[graph mig 1]: graph/db: migrate graph nodes from kvdb to SQL #10036
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
[graph mig 1]: graph/db: migrate graph nodes from kvdb to SQL #10036
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.
Summary of Changes
Hello @ellemouton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request initiates the significant effort to migrate the graph store from its current KV database backend to a new SQL database. It establishes the core migration framework and successfully migrates all existing graph nodes and the designated source node, ensuring data integrity through rigorous validation and comparison. This is the first in a series of changes aimed at transitioning the graph data storage.
Highlights
- Graph Database Migration Framework: Introduces the foundational
MigrateGraphToSQL
function and supporting helpers (checkGraphExists
,compare
) to facilitate the transition of the graph store from a KV database to a SQL database. This function is currently only called via tests and will be integrated into the mainlnd
binary once the full migration is complete. - Node and Source Node Migration: Implements the first phase of the graph migration, specifically handling the transfer of all existing graph nodes and the designated source node from the KV backend to the new SQL schema. This includes robust validation of node data, such as skipping nodes with invalid TLV streams, and post-migration verification to ensure data integrity.
- Comprehensive Migration Testing: Adds a dedicated and extensive test suite (
sql_migration_test.go
) to thoroughly validate the migration logic. This includes tests for various node and source node scenarios, edge cases like malformed data, and a utility for testing against real-worldchannel.db
orchannel.sqlite
files (though this specific test is skipped by default). - Test Infrastructure Refinement: Refactors the database test setup by introducing a
newBatchQuerier
helper in bothtest_postgres.go
andtest_sqlite.go
. A newtest_sql.go
file centralizes theNewTestDB
function, streamlining the creation of SQL-backed test environments and improving modularity.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces the framework for migrating the graph store from a KV backend to a SQL backend. It includes the migration logic for nodes and the source node, along with comprehensive tests. The code is well-structured and the migration process is robust, with verification steps to ensure data integrity.
Fix a typo in the build directive.
Factor out the transaction executor construction so that we can have access to the raw BatchedSQLQueries type from within tests.
ab4a75e
to
0c03a49
Compare
|
||
// TODO(elle): At this point, we should check the loaded node | ||
// to see if we should extract any DNS addresses from its | ||
// opaque type addresses. This is expected to be done in: | ||
// https://github.com/lightningnetwork/lnd/pull/9455. | ||
// This TODO is being tracked in | ||
// https://github.com/lightningnetwork/lnd/issues/9795 as this | ||
// must be addressed before making this code path active in | ||
// production. |
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.
fyi @mohamedawnallah: this is where the migration will happen for DNS adds currently stored as opaque addrs - which is why we dont need that migration to happen in #9455.
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.
Awesome test framework! LGTM 🎉
name: "nodes", | ||
write: writeUpdate, | ||
//nolint:ll | ||
objects: []any{ |
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.
Very nice test framework!
I wonder if it would make sense to use the rapid
library to generate some random nodes here as well?
Not sure how much setup code that would require though, so just a thought and definitely non-blocking.
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.
Or perhaps we could use something like this to produce a certain amount of random objects in each test?
https://github.com/lightninglabs/taproot-assets/blob/7ce15016366c38df1670ce6e1928ffa4b30fe9b7/internal/test/copy.go#L15
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.
Same comment, perhaps worth considering (see example in invoices migration tests).
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.
cool, will do - gonna address this in a follow up as a separate test that covers nodes/channels/channel policies just to keep this test deterministic. Added tracking for this in the epic
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.
LGTM 🎉
name: "nodes", | ||
write: writeUpdate, | ||
//nolint:ll | ||
objects: []any{ |
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.
Same comment, perhaps worth considering (see example in invoices migration tests).
In this commit, the basic framework for the graph SQL migration is added. This sets us up for the commits to follow which will add migration logic for each table in the graph commit by commit.
This commit was inspired by the invoices package TestMigrationWithChannelDB test helper. This test is purely for running locally to test the graph SQL migration. It allows a dev to run the migration against a local `channel.sqlite` or even `channel.db` file.
0c03a49
to
f4c4605
Compare
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.
ultra speed review! thanks yall!
name: "nodes", | ||
write: writeUpdate, | ||
//nolint:ll | ||
objects: []any{ |
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.
cool, will do - gonna address this in a follow up as a separate test that covers nodes/channels/channel policies just to keep this test deterministic. Added tracking for this in the epic
So that we can use it for comparisons in other migrations too.
f4c4605
to
f13874a
Compare
cc @guggero for override merge 🙏 |
oops - looks like i didnt push the changes properly - will update soon! |
f13874a
to
d62aa8f
Compare
This commit expands MigrateGraphToSQL so that it migrates all the graph nodes from kvdb to SQL. The TestMigrateGraphToSQL test is updated to cover the basic LightningNode cases. A new test, `TestSQLMigrationEdgeCases`, is also added and a case is added to tests the edge case where a node exists in our kvdb store that has invalid TLV bytes.
d62aa8f
to
cb959bd
Compare
This PR adds the framework for the migration function and main migration test of the graph store.
It then adds the first part of the migration which migrates the nodes & source node.
Please see #10025 for the final result we are aiming for here.
Part of #9795