Replication state #60
Conversation
|
This will need some adjustments if #61 is the way forward. Also maybe we can brainstorm how this might work for the blob store? I wrote an initial implementation in #62. I think we might need to handle it differently because replication being "complete" is not as simple for the blob store - for other stores our aim is to sync everything, so when the peer and local have everything shared, we consider replication "done". However with the blobs we will only be syncing previews and thumbnails from other peers (not originals) when syncing phones, and in the future will have more complicated selective sync. Also we likely want to track "files" rather than blocks (since blobs are split into multiple blocks). I can see a way to do this for "download" progress, but tracking uploads to a peer and whether those are "done" is more complicated. Maybe we could listen to and track peer "wants"? Some joint thinking on this could be helpful. |
sethvincent
force-pushed
the
replication-state
branch
from
07e5275 to
e63980d
Compare
sethvincent
force-pushed
the
replication-state
branch
from
06938df to
52c77d8
Compare
| @@ -40,3 +42,37 @@ export function replicate(cm1, cm2) { | |||
| ]) | |||
| } | |||
| } | |||
|
|
|||
| export async function waitForCores (coreManager, keys) { | |||
There was a problem hiding this comment.
I moved these functions into the core-manager helpers file. Haven't imported them into the core-manager tests yet but can do that.
| if (hasKeys(keys, allKeys)) return | ||
| return new Promise(res => { | ||
| coreManager.on('add-core', async function onAddCore ({ key, core }) { | ||
| await core.ready() |
There was a problem hiding this comment.
Added await core.ready() here for convenience. Did a quick check to make sure that still works for the core-manager tests and it seems fine.
tests/helpers/core-manager.js
Outdated
| } = {}) { | ||
| const db = new Sqlite(':memory:') | ||
| const keyManager = new KeyManager(rootKey) | ||
| return new CoreManager({ | ||
| db, | ||
| keyManager, | ||
| storage: RAM, | ||
| projectKey | ||
| projectKey, | ||
| projectSecretKey, |
There was a problem hiding this comment.
I found it was necessary to be able to pass the secret key here.
|
I have a few tests to add still but otherwise this is ready for review. |
|
Fixed the weird stalling test 🎉 |
There was a problem hiding this comment.
I haven't read through the tests yet, but reviewed sync/replication-state.js. I wouldn't normally comment on performance questions, but I think a lot of this code is in "hot" code paths, that are going to be called multiple times. In general I think it is better for perf to directly use iterators in for...of statements, rather than converting them to arrays and then using the array iteration methods (map, reduce, filter). I believe converting to an array is already going to iterate through them once, and then has the memory overhead of creating the array, which then needs to be garbage collected.
Also, while I am a Array.reduce() fan, I keep seeing feedback that people find it hard to understand code that uses reduce(), so maybe using for...of can be better for readability too.
| return obj | ||
| }, {}) | ||
|
|
||
| const synced = this.isSynced() |
There was a problem hiding this comment.
Because this getter might be accessed frequently, maybe just copy-paste the isSynced() code into the loop/reducer above, to avoid iterating through the states twice.
Co-authored-by: Gregor MacLennan <gmaclennan@digital-democracy.org>
Co-authored-by: Gregor MacLennan <gmaclennan@digital-democracy.org>
Closes #51.
This pr provides progress tracking of replication between a core and its peers.
Based largely on https://github.com/digidem/hypercore-replication-progress/