graph: extract cache from CRUD [2] #9545
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 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 (
|
ellemouton
force-pushed
the
graph13
branch
2 times, most recently
from
9be1761 to
1efaeca
Compare
| return g, nil | ||
| } | ||
|
|
||
| // setGraphCache sets the KVStore's graphCache. | ||
| // | ||
| // NOTE: this is temporary and will only be called from the ChannelGraph's |
| *KVStore | ||
| } | ||
|
|
||
| // NewChannelGraph creates a new ChannelGraph instance with the given backend. | ||
| func NewChannelGraph(cfg *Config) (*ChannelGraph, error) { | ||
| func NewChannelGraph(cfg *Config, options ...ChanGraphOption) (*ChannelGraph, |
|
|
||
| // Config is a struct that holds all the necessary dependencies for a | ||
| // ChannelGraph. | ||
| type Config struct { |
There was a problem hiding this comment.
nit: KVDBConfig? No strong opinion here tho, as this is a place I find myself struggling sometimes. It's nice to just name it Config, as suggested by the official go, and use it as graphdb.Config. But in practice it's easier to search using a unique name.
There was a problem hiding this comment.
so this is the config struct for ChannelGraph and not for the KVStore. It just at the moment happens to only hold KVStore related things but will be expanded in future.
| o(opts) | ||
| } | ||
|
|
||
| store, err := NewKVStore(cfg.KVDB, cfg.KVStoreOpts...) |
There was a problem hiding this comment.
it looks like NewKVStore can just take the Config, maybe in another PR.
There was a problem hiding this comment.
see above comment about Config struct being ChannelGraph config and not KVStore config.
We could add a KVStore config struct and then have the COnfig struct hold that struct but i think we can also after this series unexport anything KVStore related anyways so then would probs not make sense to have an exported KVStore config struct
Namespace these options so that we can introduce separate options for the new ChannelGraph.
And use this struct to pass NewChannelGraph anything it needs to be able to init the KVStore that it houses. This will allow us to add ChannelGraph specific options.
In this commit, we let the ChannelGraph be responsible for populating the graphCache and then passing it to the KVStore. This is a first step in moving the graphCache completely out of the KVStore layer.
|
|
||
| // Config is a struct that holds all the necessary dependencies for a | ||
| // ChannelGraph. | ||
| type Config struct { |
There was a problem hiding this comment.
so this is the config struct for ChannelGraph and not for the KVStore. It just at the moment happens to only hold KVStore related things but will be expanded in future.
| o(opts) | ||
| } | ||
|
|
||
| store, err := NewKVStore(cfg.KVDB, cfg.KVStoreOpts...) |
There was a problem hiding this comment.
see above comment about Config struct being ChannelGraph config and not KVStore config.
We could add a KVStore config struct and then have the COnfig struct hold that struct but i think we can also after this series unexport anything KVStore related anyways so then would probs not make sense to have an exported KVStore config struct
ellemouton
merged commit 83ed7df
into
lightningnetwork:elle-graphCacheBase
** Part 2 **
This builds towards this final result by building this side branch incrementally.
In this PR, we first rename the existing KV store functional option type to be more context specific so that we
free up the more general
Optionsname for the newChannelGraph.Then, we let the ChannelGraph init the KVStore using its config options.
Finally, we let the ChannelGraph be responsible for the init of the graphCache. For now, both structs hold a pointer to the
graphCache. But the end of the series, the KVStore will no longer have a pointer to the graphCache