feat: metadata book #638
feat: metadata book #638
Conversation
src/peer-store/index.js
Outdated
| protocols: [] | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Couldn't this function just get a union of all the keys from each book and then do a this.get(peerId) on that? This would simplify data retrieval and remove the need for the strict ordering that's happening here.
There was a problem hiding this comment.
This got definitely cleaner! thanks :)
There was a problem hiding this comment.
These are all maps so in theory you could simplify it further to:
const storedPeers = new Set([
...this.addressBook.data.keys(),
...this.keyBook.data.keys(),
...this.protoBook.data.keys(),
...this.metadataBook.data.keys(),
])Also, protobook is listed twice and keybook is missing.
There was a problem hiding this comment.
That's true! Looking into it
| @@ -58,6 +65,7 @@ class PersistentPeerStore extends PeerStore { | |||
| this.on('change:protocols', this._addDirtyPeer) | |||
| this.on('change:multiaddrs', this._addDirtyPeer) | |||
| this.on('change:pubkey', this._addDirtyPeer) | |||
| this.on('change:metadata', this._addDirtyPeerMetadata) | |||
There was a problem hiding this comment.
Why don't we do the same thing as the other events and just mark the peer itself as dirty, then get the latest when we go to commit?
There was a problem hiding this comment.
There is one issue with that approach, that I cannot circumvent unless I add the extra dirty record.
While in the other books we know that if something changed and it is not in the book anymore, it is deleted, in the metadataBook is not that simple. So, for adding it is simple, but for deleting we would not know the metadata keys that were deleted (because they were simply gone from the book), which would result in metadata being persisted and not removed anymore. When we do the restart, that metadata would be around.
There was a problem hiding this comment.
Fair enough, we could check the datastore keys and then delete the ones that we no longer have, but this might have some drawbacks to shutdown storage time (extra datastore round trips). We can punt this decision for now, as there's no user facing impact with this.
vasco-santos
force-pushed
the
feat/metadata-book
branch
2 times, most recently
from
2ec7628
to
60afd73
Compare
vasco-santos
force-pushed
the
feat/metadata-book
branch
from
60afd73
to
ee2406f
Compare
src/peer-store/index.js
Outdated
| protocols: [] | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
These are all maps so in theory you could simplify it further to:
const storedPeers = new Set([
...this.addressBook.data.keys(),
...this.keyBook.data.keys(),
...this.protoBook.data.keys(),
...this.metadataBook.data.keys(),
])Also, protobook is listed twice and keybook is missing.
* feat: metadata book * chore: address review * chore: address review
* feat: metadata book * chore: address review * chore: address review
This PR adds the metadataBook to the PeerStore as described in #627
Closes #627