-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[13] graph/db: SQL-ize the zombie index #9937
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
[13] graph/db: SQL-ize the zombie index #9937
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@bhandras: review reminder |
56d3be6
to
f09565e
Compare
f09565e
to
5efc000
Compare
(note: data race unrelated & is fixed here) |
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.
Super clean! Just one thing I think we should fix (type of scid
column).
5efc000
to
5a7cf6b
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.
LGTM 🎉
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! 🎉
return nil | ||
}, sqldb.NoOpReset) | ||
if err != nil { | ||
return false, route.Vertex{}, route.Vertex{} |
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.
nit: could you pls add a TODO to return the error in the future?
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 yeah def - good catch. Will add TODO & make a note in the epic & fix by end of series 👍
graph/db/sql_store.go
Outdated
return nil | ||
}, sqldb.NoOpReset) | ||
if err != nil { | ||
return err |
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.
nit: should we also decorate the error like we do below in NumZombies
? Perhaps this could be a good practice in general due to the opaque nature of SQL errors. wdyt? (same above for MarkeEdgeZombie)
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.
indeed 🫡 added
graph/db/sql_store.go
Outdated
err := s.db.ExecTx(ctx, sqldb.WriteTxOpt(), func(db SQLQueries) error { | ||
for _, chanID := range chanIDs { | ||
var chanIDB [8]byte | ||
byteOrder.PutUint64(chanIDB[:], chanID) |
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.
nit: since this pattern is used everywhere, perhaps we could create a small helper function for 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.
good idea - added a commit
So that we can use it in our SQLStore implementation too without needing to fetch the entire ChannelEdgePolicy.
Note that this table will only contain entries for channels that we have deleted from the `channels` table which is why we cannot use foreign keys. Similarly, we may no longer have node entries for the nodes in the table.
Here we implement the SQLStore methods: - MarkEdgeZombie - MarkEdgeLive - IsZombieEdge - NumZombies These will be tested in the next commit as one more method implementation is required.
This lets us run TestGraphZombieIndex against the SQL backends.
5a7cf6b
to
2a6e668
Compare
This PR adds the schema, queries and CRUD for the zombie index.
The following methods are implemented:
This then lets us run
TestGraphZombieIndex
against the SQL backends.Part of #9795
Depends on #9936
See #9932 for the full picture that we are aiming at