multi: create split local/remote database structure #4348
Conversation
Roasbeef
force-pushed
the
etcd-local-graph-db
branch
from
3f7b8b9
to
0d566f2
Compare
There was a problem hiding this comment.
nice changes, surprised how small the diff is! only thing that comes to mind is that graph info about our local chans will be stored in the non-replicated database, so if we lose that we won't be able to reconstruct or update our own channel with our peer or to the wider network.
in the past i looked into the idea of "source hints", which act like hop hints but connect your local node the rest of the channel graph (rather than the connecting the private destination). these are computed from the replicated storage and fed in-memory into path finding to allow the split to work properly. that was over a year ago tho and the router has changed significantly since then, so that would need to be rewritten almost entirely.
chainregistry.go
Outdated
| @@ -220,7 +220,7 @@ func newChainControlFromConfig(cfg *Config, chanDB *channeldb.DB, | |||
| var err error | |||
|
|
|||
| // Initialize the height hint cache within the chain directory. | |||
| hintCache, err := chainntnfs.NewHeightHintCache(chanDB) | |||
| hintCache, err := chainntnfs.NewHeightHintCache(remoteDB) | |||
There was a problem hiding this comment.
would think that height hints belong in a local db, they are not critical data and can be recomputed. in the past we've had cases where we wanted people to be able to delete their source hints to force a rescan
| @@ -323,7 +325,8 @@ func noiseDial(idKey keychain.SingleKeyECDH, | |||
|
|
|||
| // newServer creates a new instance of the server which is to listen using the | |||
| // passed listener address. | |||
| func newServer(cfg *Config, listenAddrs []net.Addr, chanDB *channeldb.DB, | |||
| func newServer(cfg *Config, listenAddrs []net.Addr, | |||
| localChanDB, remoteChanDB *channeldb.DB, | |||
There was a problem hiding this comment.
one day we'll make a Config struct..
server.go
Outdated
| sharedSecretPath := filepath.Join(cfg.localDatabaseDir(), "sphinxreplay.db") | ||
| // the same directory as the channel graph database. We don't need to | ||
| // replicate this data, so we'll store it locally. | ||
| graphDir := localChanDB.Path() |
There was a problem hiding this comment.
note: eventually we could remove the path from the channeldb. I think apart from here it's only used in tests.
Roasbeef
force-pushed
the
etcd-local-graph-db
branch
from
0d566f2
to
0de3359
Compare
Roasbeef
requested review from
bhandras and
cfromknecht
and removed request for
halseth
| // initializeDatabases extracts the current databases that we'll use for normal | ||
| // operation in the daemon. Two databases are returned: one remote and one | ||
| // local. However, only if the replicated database is active will the remote | ||
| // database point to a unique database. Otherwise, the local and remote DB will |
There was a problem hiding this comment.
the local and remote DB will both point to the same local database.
since RemoteDB isn't necessarily remote, I could see this naming being pretty confusing. might consider using location-independent names like GraphDB and ChanDB/SafuDB/EverythingElseDB to reflect what is stored in the databases. the rest of the codebase doesn't really care where the data is being stored (by design)
There was a problem hiding this comment.
With the change to move the height hints to the local DB, GraphDB doesn't quite work here. I agree in general though, as then the "functional" splits let us start to abstract away those portions, so being able to manipulate the graph on an interface level which'll let us use more specific DB features rather than mapping everything unto the kvdb interface. In any case I don't think this is blocking, since we're not exposing any new config options, they're (for now) just internal variable names.
|
@Roasbeef still targeting 0.11? |
Roasbeef
force-pushed
the
etcd-local-graph-db
branch
from
0de3359
to
b7b4279
Compare
Fixed! Was a pathing issue, so all the nodes tried to open the same sphinx replay DB. |
There was a problem hiding this comment.
LGTM, I think this is a great first step towards data separation. 💾
Some last comments (nb for this PR):
- I agree with @cfromknecht that it would be desirable to have functional separation rather than local/remote as that'd allow us to think about design more clearly.
- As a first step towards that larger goal, perhaps we could introduce interfaces for cleaner separation of local/remote functionality. This would also be helpful for future contributors by enforcing some level of correctness at compile time.
Roasbeef
force-pushed
the
etcd-local-graph-db
branch
from
b7b4279
to
749d103
Compare
Roasbeef
force-pushed
the
etcd-local-graph-db
branch
2 times, most recently
from
b7486fd
to
47bec5f
Compare
In this commit, we modify the existing `GetBackend` method to now be called `GetBackends`. This new method will populate a new `RemoteDB` attribute based on if the replicated backend is active or not. As is, the local backend is used everywhere. An upcoming commit will once again re-enable the remote backend, in a hybrid manner.
In this commit, we split the database storage into two classes: remote and local data. If etcd isn't active, then everything is actually just local though we use two pointers everywhere. If etcd is active, then everything but the graph goes into the remote database.
Roasbeef
force-pushed
the
etcd-local-graph-db
branch
from
47bec5f
to
f58b00e
Compare
In this follow up PR to the recent
etcdbackend PR, we modify our data storage to support a new hybrid system. For operations that require lower read latency, or which read data sources which can easily be re-populated (the graph as an example), we use a local bolt option. If a replicated database is active (in this caseetcd), then we'll use that replicated database for just about everything else.Admittedly, the current implementation is a bit crude in that it creates two new backends
LocalDBandRemoteDB, which are then passed into the appropriate context based on the replication needs of the data. This results in two new pointers in the server, which will force the caller to decide on if the data should remain local for low latency access, or should be replicated along with other state like the channel state machine, payments, etc.