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

autorelay: Split libp2p.EnableAutoRelay into 2 functions #2022

Merged
merged 5 commits into from Feb 7, 2023

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Jan 27, 2023

Provide two specific ways to enable the autorelay subsystem libp2p.EnableAutoRelayWithStaticRelays
libp2p.EnableAutoRelayWithPeerSource

fixes: #1866

Provide two specific ways to enable the autorelay subsystem
libp2p.EnableAutoRelayWithStaticRelays
libp2p.EnableAutoRelayWithPeerSource
@p-shahi p-shahi changed the title Split libp2p.EnableAutoRelay into 2 functions autorelay: Split libp2p.EnableAutoRelay into 2 functions Feb 2, 2023
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

One questions about the API. There's also a failing staticcheck check.

options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved

// WithMinInterval sets the minimum interval after which peerSource callback will be called for more
// candidates even if AutoRelay needs new candidates
func WithMinInterval(interval time.Duration) Option {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the minimum interval both a parameter to WithPeerSource and a separate option here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to fix that, just wanted to confirm once if it's fine to change WithPeerSource method signature.

#1866 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed. Removed from WithPeerSource

@sukunrt sukunrt requested review from marten-seemann and removed request for MarcoPolo February 6, 2023 08:15
@sukunrt
Copy link
Member Author

sukunrt commented Feb 6, 2023

@marten-seemann addressed review comments.
I've removed the test TestIncorrectInit in autorelay_test.go since that should be fixed by this PR and we shouldn't need this test that checks for panic on incorrect initialisation.

@marten-seemann marten-seemann linked an issue Feb 6, 2023 that may be closed by this pull request
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

LGTM. @MarcoPolo, could you have a look at this PR please? Just want to make sure we definitely get the API right this time.

@MarcoPolo
Copy link
Contributor

Pushed a change to make the PeerSource function a separate type so that it can be documented properly

p2p/host/autorelay/options.go Outdated Show resolved Hide resolved
p2p/host/autorelay/options.go Outdated Show resolved Hide resolved
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@marten-seemann marten-seemann merged commit f9c02c1 into libp2p:master Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify autorelay.WithPeerSource autorelay: refactor libp2p.EnableAutoRelay option
3 participants