[13] graph/db: SQL-ize the zombie index#9937
[13] graph/db: SQL-ize the zombie index#9937ellemouton merged 5 commits intolightningnetwork:masterfrom
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) |
guggero
left a comment
There was a problem hiding this comment.
Super clean! Just one thing I think we should fix (type of scid column).
5efc000 to
5a7cf6b
Compare
| return nil | ||
| }, sqldb.NoOpReset) | ||
| if err != nil { | ||
| return false, route.Vertex{}, route.Vertex{} |
There was a problem hiding this comment.
nit: could you pls add a TODO to return the error in the future?
There was a problem hiding this comment.
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.
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)
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.
nit: since this pattern is used everywhere, perhaps we could create a small helper function for it.
There was a problem hiding this comment.
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
TestGraphZombieIndexagainst the SQL backends.Part of #9795
Depends on #9936
See #9932 for the full picture that we are aiming at