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

add an option to start the relay v2 #1197

Merged
merged 1 commit into from
Sep 29, 2021
Merged

add an option to start the relay v2 #1197

merged 1 commit into from
Sep 29, 2021

Conversation

marten-seemann
Copy link
Contributor

I'm not satisfied with the names for the config options, but I struggle to come up with something better. Suggestions would be highly appreciated.

I wish we could use EnableRelay() here, but unfortunately this is already taken to enable the relay transport.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Yeah, these names are very confusing.

I have suggested some alternatives.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
@marten-seemann
Copy link
Contributor Author

RelayService sounds good to me! Will update the PR.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

LGTM; only thing we need is to make it disabled by default as @Stebalien requested.
Also, the package name for the manager/service is rather confusing, let's call it something more appropriate.

config/config.go Outdated
Relay bool
Relay bool // should the relay transport be used

DisableRelayService bool // should we run a circuitv2 relay (if publicly reachable)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's heed to @Stebalien's request to not enable it by default, so this should be EnableRelayService I guess and the logic inverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.
We'll still enable this by default in IPFS / Filecoin, won't we?

Copy link
Member

Choose a reason for hiding this comment

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

We'll still enable this by default in IPFS / Filecoin, won't we?

Yes, or at least in IPFS. But libp2p is a library not an application.

options.go Outdated Show resolved Hide resolved
p2p/host/relayv2/relay.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann changed the title start a relay v2 by default, add config options add an option to start the relay v2, add config options Sep 25, 2021
@marten-seemann marten-seemann changed the title add an option to start the relay v2, add config options add an option to start the relay v2 Sep 25, 2021
@marten-seemann marten-seemann force-pushed the use-relayv2 branch 2 times, most recently from 3c26307 to 70e17db Compare September 27, 2021 12:44
@marten-seemann marten-seemann merged commit 3e1f668 into master Sep 29, 2021
@marten-seemann marten-seemann deleted the use-relayv2 branch September 29, 2021 14:20
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

3 participants