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

network: unify wsPeerCore use for HTTP and p2p HTTP transport #5933

Merged

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Feb 8, 2024

Summary

wsPeerCore refactoring aiming to make HTTPPeer to use "get path and fire request` approach rather than analyzing peer/url type.

Before:
wsPeerCore has rootURL field that is used for logging, peer identification and HTTP connectivity. Its constructor also accepts a RoundTripper (rate limiting transport) to construct a default http.Client.
Adding p2p into the mix changed the latest part - HTTP connectivity is handled by pre-created p2p http transport (and client) making the rootURL to serve a dual purpose (peer id as a full p2p multiaddr string, and a peer type separation field in HTTP clients code)

After:
wsPeerCore still has rootURL for id and connectivity but the connectivity bit is handled by pre-constructed http.Client that uses rootURL to build up a full URL. This allowed to unify peers with http and p2p transport from client's viewpoint.

Test Plan

  1. Added a unit test for the new wrapper HTTPPAddressBoundTransport type
  2. Existing tests should pass.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (472f01b) 56.08% compared to head (f0955ce) 56.06%.

Files Patch % Lines
network/wsNetwork.go 85.29% 5 Missing ⚠️
network/hybridNetwork.go 0.00% 4 Missing ⚠️
network/p2pNetwork.go 66.66% 2 Missing ⚠️
network/netprio.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           feature/p2p    #5933      +/-   ##
===============================================
- Coverage        56.08%   56.06%   -0.02%     
===============================================
  Files              481      481              
  Lines            68072    68053      -19     
===============================================
- Hits             38180    38157      -23     
- Misses           27281    27290       +9     
+ Partials          2611     2606       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@algorandskiy algorandskiy changed the title network: separate peer address from peer url network: unify wsPeerCore use for HTTP and p2p HTTP peers Feb 8, 2024
@algorandskiy algorandskiy changed the title network: unify wsPeerCore use for HTTP and p2p HTTP peers network: unify wsPeerCore use for HTTP and p2p HTTP transport Feb 8, 2024
@algorandskiy algorandskiy marked this pull request as ready for review February 8, 2024 20:49
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

In general, I think this is an improvement so am marking approved. It would be nice if there was a rich type for address that handled whether its for http or multiaddr at construction (making it a property of a struct, removing some checks, etc) but not a show stopper.

@@ -92,7 +92,7 @@ type GossipNode interface {

// GetHTTPClient returns a http.Client with a suitable for the network Transport
// that would also limit the number of outgoing connections.
GetHTTPClient(peer HTTPPeer) (*http.Client, error)
GetHTTPClient(address string) (*http.Client, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any benefit to consider around using a stronger type than string here? I.E. enforcing formatting etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this in previous PR, HTTPPeer type was needed only to get its address to reconstruct p2p transport. both ws and p2p implementations fail on parsing so I think it is OK

@algorandskiy
Copy link
Contributor Author

Also thought about wrapping address - looked a bit bigger change, maybe in subsequent PRs as needed.

@algorandskiy algorandskiy merged commit e506ffd into algorand:feature/p2p Feb 8, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants