ACK feature #215
Merged
ACK feature #215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martinheidegger
approved these changes
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. |
This was referenced Jul 30, 2019
|
Updated this patch to depend on hypercore-protocol/hypercore-protocol#37 so the CI ought to fail until that gets merged upstream. |
mafintosh
reviewed
Aug 7, 2019
lib/replicate.js
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
this is stored at peer.stream.stream already (could be named better yes)
|
7.6.0 |
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #130 but adds:
'ack'event on the replication stream instead of emitting events through the peer instance (only available by listening onfeed.on('peer-add', fn)and the peer instance isn't an EventEmitter in mainline anyways)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.