refactor: Add peerstore to multistore #1980
Conversation
fredcarle
added
area/datastore
datastore/multi.go
Outdated
| root: rootRW, | ||
| data: prefix(rootRW, dataStoreKey), | ||
| head: prefix(rootRW, headStoreKey), | ||
| peer: namespace.Wrap(rootstore, rootStoreKey), |
There was a problem hiding this comment.
question: Why is the peerstore not prefixed like the rest of the stores?
There was a problem hiding this comment.
I pushed the documentation of this at the same time you commented.
There was a problem hiding this comment.
😁 I thought I had gone mad/blind when I refreshed to add another comment and saw it lol.
question: Why do we want to rely on the libp2p peerstore to assign the prefix?
There was a problem hiding this comment.
Because it's hard coded in the libp2p pstoreds package. If we assign a prefix of peers here we end up with what we had before /db/peers/peers/addr/...
There was a problem hiding this comment.
Might it be safer to prefix it with extern or something then, instead of giving an external package full access to the root store?
e.g.
/db/extern/peers/addr/...
/db/extern/foo/bar/ // I have no idea what else we might use this for, hopefully such cases are rare.
There was a problem hiding this comment.
That's a fair observation. What if we make it specific? db/ps/peers/...
There was a problem hiding this comment.
Totally happy with that :) I know it seems unlikely to ever cause problems, but it feels a bit wrong to rely on just giving it the root.
fredcarle
force-pushed
the
fredcarle/refactor/I1680-peerstore-in-multistore
branch
from
3754d13
to
c5a95c6
Compare
| dataStoreKey = rootStoreKey.ChildString("/data") | ||
| headStoreKey = rootStoreKey.ChildString("/heads") | ||
| blockStoreKey = rootStoreKey.ChildString("/blocks") | ||
| rootStoreKey = ds.NewKey("db") |
There was a problem hiding this comment.
question: Is this a bug fix? Were we adding // to our keys instead of / here?
There was a problem hiding this comment.
ChildString does NewKey(k.string + "/" + s) and the NewKey function adds a / prefix if none are present and cleans the // that would have been present. Removing the / from the passed param makes it more "correct" according to what the function is doing.
There was a problem hiding this comment.
Ah nice - thanks Fred :)
Codecov ReportAttention:
@@ Coverage Diff @@
## develop #1980 +/- ##
===========================================
- Coverage 74.76% 74.54% -0.22%
===========================================
Files 246 246
Lines 24298 24306 +8
===========================================
- Hits 18166 18118 -48
- Misses 4918 4989 +71
+ Partials 1214 1199 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -66,7 +66,7 @@ func NewTxnFrom(ctx context.Context, rootstore ds.TxnDatastore, id uint64, reado | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| multistore := MultiStoreFrom(rootTxn) | |||
| multistore := MultiStoreFrom(ShimTxnStore{rootTxn}) | |||
There was a problem hiding this comment.
question: Why have you made this change? There should be no need for it root is IterableTxnDatastore
There was a problem hiding this comment.
IterableTxnDatastore does not implement Close and MultiStoreFrom now requires a ds.Datastore.
There was a problem hiding this comment.
Ah okay - thanks for clarifying :)
fredcarle
force-pushed
the
fredcarle/refactor/I1680-peerstore-in-multistore
branch
from
5c4dedf
to
4ec2aff
Compare
fredcarle
deleted the
fredcarle/refactor/I1680-peerstore-in-multistore
branch
## Relevant issue(s) Resolves sourcenetwork#1680 ## Description This small refactor adds the `peerstore` to the `multistore`. In doing so, we broaden the types of stores that can be part of the `multistore` by starting from a `ds.Datastore` instead of the more restrictive `DSReaderWriter`
Relevant issue(s)
Resolves #1680
Description
This small refactor adds the
peerstoreto themultistore. In doing so, we broaden the types of stores that can be part of themultistoreby starting from ads.Datastoreinstead of the more restrictiveDSReaderWriterTasks
How has this been tested?
make test showed no change in behaviour.
Specify the platform(s) on which this was tested: