-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: implement peering service #7362
Conversation
Depends on ipfs/go-ipfs-config#96 |
@@ -703,6 +706,26 @@ intentionally re-using the real message's message ID. | |||
|
|||
Default: `false` | |||
|
|||
### `Peering` |
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.
include expected behavior / semantics for how this behaves when one of the two peers has it, and when both have it mutually.
peering/peering.go
Outdated
nextDelay time.Duration | ||
} | ||
|
||
func (ph *peerHandler) stop() { |
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.
cancel ph.ctx ?
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.
Seems reasonable to me, I left a few comments/change requests but nothing super dramatic in case you want to just merge this and come back to it later.
- [`Peering`](#peering) | ||
- [`Peering.Peers`](#peeringpeers) |
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.
should any of this behavior going under the experimental label or no?
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 figured it was a minimal enough feature that that wasn't really necessary. I want people to start using this feature. Thoughts?
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.
But we can revisit this decision once the RC has shipped.
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.
Seems fine, I wasn't really sure what qualified as experimental vs not which is why I asked 😄
MVP for #6097 This feature will repeatedly reconnect (with a randomized exponential backoff) to peers in a set of "peered" peers. In the future, this should be extended to: 1. Include a CLI for modifying this list at runtime. 2. Include additional options for peers we want to _protect_ but not connect to. 3. Allow configuring timeouts, backoff, etc. 4. Allow groups? Possibly through textile threads. 5. Allow for runtime-only peering rules. 6. Different reconnect policies. But this MVP should be a significant step forward.
Co-authored-by: Will <[email protected]>
Co-authored-by: Adin Schmahmann <[email protected]>
* better name for timer * cancel context from within stop
* Explain _why_ it exists. * Explain how it can be used.
While preserving some randomness. And add a test.
@aschmahmann could you give this a once over post merge? I think it's good enough to go in the RC, but I'd like you to look at my changes. |
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.
@Stebalien left a small comment on the go test, but LGTM
return h1.Network().Connectedness(h2.ID()) == network.Connected | ||
}, 30*time.Second, 100*time.Millisecond) | ||
|
||
require.Len(t, h1.Network().Peers(), 3) |
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 don't know if it's worth doing anything about this, but noting that there's an unlikely race with the connmgr here where we prune in between connecting and doing the size check.
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 set the high water to 100 so that shouldn't happen.
MVP for #6097
This feature will repeatedly reconnect (with a randomized exponential backoff) to peers in a set of "peered" peers.
In the future, this should be extended to:
But this MVP should be a significant step forward.