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

ACK feature #215

Merged
merged 4 commits into from Aug 7, 2019

Conversation

@substack
Copy link
Contributor

commented Jul 5, 2019

This PR is based on #130 but adds:

  • updated to the latest hypercore
  • adds many tests
  • uses an 'ack' event on the replication stream instead of emitting events through the peer instance (only available by listening on feed.on('peer-add', fn) and the peer instance isn't an EventEmitter in mainline anyways)
  • sets the length in an acknowledgement HAVE to 0

The original technique in the ACK PR mostly works but creates a bit of extra noise with false positives. This extra noise would probably not be too much of an issue in practice for many use cases because any ACK'd records would also be present on the remote machine (if the remote is being honest). But setting the length to 0 for ACKs avoids this noise. A better option might be to add a special type for ACK messages, but this patch works with that is currently available in hypercore-protocol.

There are some legacy cases for length=0 in HAVEs but the ACK cases are narrow and only reachable when ACK is enabled and a bitfield is not set on the HAVE, which the legacy branch seems to expect.

If this patch is accepted it would be good to document the convention it depends on, where a HAVE with length=0 and no bitfield is considered an ACK. This also means that ACKs are limited to acknowledging individual sequences, not whole ranges at a time (since the length field is set to zero). Negative values could be used, but at that point I think it would be better to have a dedicated ACK message type.

@martinheidegger
Copy link
Contributor

left a comment

Documentation, tests, and a thin implementation. LveryGTM

@substack

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

I think this PR would best be redone to add an optional boolean ack field to HAVE messages in hypercore protocol instead of setting length=0 on HAVEs. This would be less hacky and would also allow for ACKs over ranges of sequences.

@substack

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Updated this patch to depend on mafintosh/hypercore-protocol#37 so the CI ought to fail until that gets merged upstream.

@@ -30,6 +30,7 @@ function replicate (feed, opts) {
var peer = new Peer(feed, opts)
peer.feed = feed
peer.stream = stream.feed(feed.key, {peer: peer})
peer._replicateStream = stream

This comment has been minimized.

Copy link
@mafintosh

mafintosh Aug 7, 2019

Owner

this is stored at peer.stream.stream already (could be named better yes)

@mafintosh mafintosh merged commit fc434cb into mafintosh:master Aug 7, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@mafintosh

This comment has been minimized.

Copy link
Owner

commented Aug 7, 2019

7.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.