-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: always end sync, even when there is missing data #96
Conversation
As a future reference for what a more complete fix might look like:
|
lib/db-sync-progress.js
Outdated
var listeners = [] | ||
|
||
multifeed.ready(function () { | ||
multifeed.feeds().forEach(onFeed) | ||
multifeed.on('feed', onFeed) | ||
stream.on('remote-feeds', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this listener on eos
below also?
progress[feed.key.toString('hex')] = feed.downloaded(0, feed.length) | ||
var total = feeds.reduce(function (acc, feed) { return acc + feed.length }, 0) | ||
var sofar = feeds.reduce(function (acc, feed) { return acc + feed.downloaded(0, feed.length) }, 0) | ||
var all = Array.from(feeds.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about this trick!
lib/db-sync-progress.js
Outdated
return acc + feed.length | ||
}, 0) | ||
var sofar = all.reduce(function (acc, feed) { | ||
return acc + feed.stats.totals.downloadedBlocks + feed.stats.totals.uploadedBlocks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might give misleading results?
My understanding of how stats.totals
works is that it records every upload and download, across multiple peer sessions. We share the same multifeed and thus the same hypercore references across all syncs, so if someone syncs with on peer they'd maybe see their uploadedBlocks
go to 100
, but on a second sync (/wo restarting Mapeo), they'll see uploadedBlocks
start at 100
.
In the v9
branch, I include the totals in the handshake, and count 'upload' and 'download' events. Maybe we just continue to track downloads only for now, but do use the 'upload' events for detection of inactivity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
Co-authored-by: Kira Oakley <[email protected]>
Co-authored-by: Kira Oakley <[email protected]>
I added a temporary hack, based on conversations with @noffle, who said that making hypercore sparse might be more to to chew than we can at the moment... a proper fix can come in v9
remote-feeds
event from multifeed v5 into multifeed v4.