swarm: make smart-dialing opt in #2340
Conversation
There was a problem hiding this comment.
I changed my mind on this being a bool. We can just allow people to inject their own dial ranker. In this release, by default a nil dial ranker will be used, disabling smart dialing. In the next release, by default the DefaultDialRanker will be used, and to disable smart dialing, you'd pass in a nil dial ranker.
defaults.go
Outdated
| @@ -135,6 +135,11 @@ var DefaultPrometheusRegisterer = func(cfg *Config) error { | |||
| return cfg.Apply(PrometheusRegisterer(prometheus.DefaultRegisterer)) | |||
| } | |||
|
|
|||
| // DefaultSmartDialing configures libp2p to disable smart dialing by default | |||
| var DefaultSmartDialing = func(cfg *Config) error { | |||
There was a problem hiding this comment.
Why do we need an option for that?
There was a problem hiding this comment.
I don't get this. This helps us flip the default option in a later release. Is there a way to do this without this?
There was a problem hiding this comment.
Do we need to explicitly define a type? Can’t we just apply the default if no dial ranker is set?
There was a problem hiding this comment.
To be clear we are now discussion the config fields: DialRanker and DialRankerCustom.
If we don't have DialRankerCustom we cannot distinguish between cases where the user has not provided a value and where the user has explicitly provided nil to disable smart dialing. This is not an issue now but for the future release where we want to turn smart dialing on by default we will need to use swarm.DefaultDialRanker in such cases. I want to keep the behaviour that option DialRanker(nil) disables smart dialing in future releases.
Using a Default Option just felt cleaner. see DefaultEnableRelay
There was a problem hiding this comment.
What about doing the thing we did for the rcmgr? Provide a NullDialRanker implementation. iiuc we already have that, the NoDelayDialRanker.
There was a problem hiding this comment.
I don't have strong opinions on this. Removed the field DialRankerCustom and the explicit default option. Now default is set here https://github.com/sukunrt/go-libp2p/blob/smart-dialing-optin/config/config.go#L180
|
@marten-seemann ignoring the changelog, is this what you had in mind? Update: removed changes to changelog in favour of #2342 |
sukunrt
force-pushed
the
smart-dialing-optin
branch
from
c32f0c0 to
1716e2e
Compare
sukunrt
force-pushed
the
smart-dialing-optin
branch
from
1716e2e to
a1cad17
Compare
|
No strong opinion, but I think it's a nicer api for the user to be able to enable/disable smart dialing with a boolean rather than having users know about dial rankers. |
No description provided.