Skip to content

fix: always end sync, even when there is missing data#96

Merged
okdistribute merged 9 commits intov8from
v8-missing
Jul 2, 2020
Merged

fix: always end sync, even when there is missing data#96
okdistribute merged 9 commits intov8from
v8-missing

Conversation

@okdistribute
Copy link
Copy Markdown
Contributor

@okdistribute okdistribute commented Jul 1, 2020

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

  1. I backported the remote-feeds event from multifeed v5 into multifeed v4.
  2. Updated sync progress to include upload as well as download events.
  3. Added a heartbeat to the sync -- every 20s, it checks to see if it has had any progress (media or db#upload or db#download) events in the past 20s. If not, it aborts the sync.

@hackergrrl
Copy link
Copy Markdown
Contributor

hackergrrl commented Jul 1, 2020

As a future reference for what a more complete fix might look like:

  • backport ensure opts from writer is used to generate hypercore kappa-db/multifeed#46 to the version of multifeed we use (pre 5.x.x I think?)
  • pass down opts.sparse = true all the way down the stack to hypercore (kappa-osm -> kappa-core -> multifeed -> hypercore)
  • figure out how to know when the multifeed handshake is done & we have all feed refs (I added this to 5.x.x, we'll need to backport)
  • figure out what blocks the remote feeds have
  • modify mapeo-core to issue core.download() requests for only those blocks
  • remove heartbeat ;)

@okdistribute okdistribute changed the title test: failing to end on missing data fix: always end sync, even when there is missing data Jul 1, 2020
multifeed.ready(function () {
multifeed.feeds().forEach(onFeed)
multifeed.on('feed', onFeed)
stream.on('remote-feeds', () => {
Copy link
Copy Markdown
Contributor

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())
Copy link
Copy Markdown
Contributor

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!

return acc + feed.length
}, 0)
var sofar = all.reduce(function (acc, feed) {
return acc + feed.stats.totals.downloadedBlocks + feed.stats.totals.uploadedBlocks
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

okdistribute and others added 5 commits July 1, 2020 17:25
Co-authored-by: Kira Oakley <noffle@users.noreply.github.com>
Co-authored-by: Kira Oakley <noffle@users.noreply.github.com>
@okdistribute okdistribute merged commit 99c9cc4 into v8 Jul 2, 2020
@okdistribute okdistribute deleted the v8-missing branch July 2, 2020 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants