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

p2p: force direct connections #2222

Merged
merged 5 commits into from
May 23, 2023
Merged

Conversation

xenowits
Copy link
Contributor

Force direct connection when there is an existing relay connection to the peer.

category: feature
ticket: #2114

@xenowits xenowits self-assigned this May 22, 2023
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.15 🎉

Comparison is base (b3d0ba0) 53.67% compared to head (3467bbd) 53.82%.

❗ Current head 3467bbd differs from pull request most recent head 08ad13d. Consider uploading reports for the commit 08ad13d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2222      +/-   ##
==========================================
+ Coverage   53.67%   53.82%   +0.15%     
==========================================
  Files         186      187       +1     
  Lines       24345    24965     +620     
==========================================
+ Hits        13066    13438     +372     
- Misses       9657     9876     +219     
- Partials     1622     1651      +29     
Impacted Files Coverage Δ
app/app.go 6.72% <0.00%> (-0.03%) ⬇️
app/lifecycle/orderstart_string.go 40.00% <ø> (ø)
cmd/relay/p2p.go 38.46% <ø> (ø)
p2p/p2p.go 22.54% <0.00%> (-3.29%) ⬇️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

p2p/p2p.go Show resolved Hide resolved
p2p/p2p.go Show resolved Hide resolved
p2p/p2p.go Outdated Show resolved Hide resolved
p2p/p2p.go Outdated Show resolved Hide resolved
p2p/p2p.go Outdated Show resolved Hide resolved
p2p/p2p.go Outdated Show resolved Hide resolved
p2p/p2p.go Outdated Show resolved Hide resolved
p2p/p2p.go Show resolved Hide resolved
@xenowits
Copy link
Contributor Author

xenowits commented May 23, 2023

Screenshot of a compose run:

Screenshot 2023-05-23 at 15 17 47

@xenowits xenowits added the merge when ready Indicates bulldozer bot may merge when all checks pass label May 23, 2023
@obol-bulldozer obol-bulldozer bot merged commit c7dc0bd into main May 23, 2023
@obol-bulldozer obol-bulldozer bot deleted the xenowits/force-direct-conn branch May 23, 2023 10:16
@OisinKyne
Copy link
Contributor

Can we write a hypothesis here to test this? e.g. how many situations do we end up stuck, and how often does the 'force' succeed? Are there any negative consequences of this?

scienceandscrewingaround

@xenowits
Copy link
Contributor Author

xenowits commented May 25, 2023

We tested this in prod starting with the core team cluster. It has a success rate of 100% for forced direct connections, you're never stuck. This force feature is not an exotic thing we do, libp2p does this a lot under the hood.

AFAIK, there are no negative consequences for this. But as with any new feature, we need to monitor this feature in production clusters from release v0.16.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants