Skip to content
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

Merged
merged 9 commits into from
Jul 2, 2020
Merged

Conversation

okdistribute
Copy link
Collaborator

@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
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
var listeners = []

multifeed.ready(function () {
multifeed.feeds().forEach(onFeed)
multifeed.on('feed', onFeed)
stream.on('remote-feeds', () => {
Copy link
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?

lib/db-sync-progress.js Outdated Show resolved Hide resolved
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
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
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
Collaborator Author

Choose a reason for hiding this comment

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

sounds good

sync.js Outdated Show resolved Hide resolved
@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