-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: extract cache from CRUD [1] #9533
graph: extract cache from CRUD [1] #9533
Conversation
Rename it to kv_store.go so that we can re-use the graph.go file name later on. We will use it to house the _new_ ChannelGraph when the existing ChannelGraph is renamed to more clearly reflect its responsibilities as the CRUD layer.
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
In this commit, we rename the existing ChannelGraph struct to KVStore to better reflect its responsibilities as the CRUD layer. We then introduce a new ChannelGraph struct which will eventually be the layer above the CRUD layer in which we will handle cacheing and topology subscriptions. For now, however, it houses only the KVStore. This means that all calls to the KVStore will now go through this layer of indirection first. This will allow us to slowly move the graph Cache management out of the KVStore and into the new ChannelGraph layer. We introduce the new ChannelGraph and rename the old one in the same commit so that all existing call-sites don't need to change at all :)
Since we have renamed a file housing some very old code, the linter has now run on all this code for the first time. So we gotta do some clean-up work here to make it happy.
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 ⚔️
@@ -269,7 +269,7 @@ type channelMapKey struct { | |||
|
|||
// getChannelMap loads all channel edge policies from the database and stores | |||
// them in a map. | |||
func (c *ChannelGraph) getChannelMap(edges kvdb.RBucket) ( | |||
func (c *KVStore) getChannelMap(edges kvdb.RBucket) ( |
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: receiver could also be renamed to s
(as in store).
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 - since we've changed this PR to point to a side branch, I'll update this as first commit of next PR if this one gets no more comments to address, otherwise will update on next push here if more review comments 🫡
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, if it is ok, perhaps as a final commit in this cache series since this receiver change will cause maximum rebase conflicts 🙃
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.
sgtm! :)
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🦾
// ChannelGraph is a layer above the graph's CRUD layer. | ||
// | ||
// NOTE: currently, this is purely a pass-through layer directly to the backing | ||
// KVStore. Upcoming commits will move the graph cache out of the KVStore and |
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.
🙏
@@ -249,6 +249,8 @@ The underlying functionality between those two options remain the same. | |||
- [Abstract autopilot access](https://github.com/lightningnetwork/lnd/pull/9480) | |||
- [Abstract invoicerpc server access](https://github.com/lightningnetwork/lnd/pull/9516) | |||
- [Refactor to hide DB transactions](https://github.com/lightningnetwork/lnd/pull/9513) | |||
- Move the graph cache out of the graph CRUD layer: |
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.
It just occurs to me that this feature is targeted for 0.20 or 0.19?
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.
depends on when we merge the base branch into master i guess! So let's leave this here for now and then if by the time we want to merge the base branch in, if 0.19 has shipped already, then i can move this to 0.20
8924d3b
into
lightningnetwork:elle-graphCacheBase
PART 1 only. See #9529 for the full thing
This is the 1st mini PR peeled off from this concept one which shows the final goal here which is to completely move the graph cache out of the KV CRUD layer.
This is part of #9494
Reminder about why we are doing this: