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

feat: inject DNS resolver #1607

Merged
merged 1 commit into from
Aug 22, 2022
Merged

feat: inject DNS resolver #1607

merged 1 commit into from
Aug 22, 2022

Conversation

Stebalien
Copy link
Member

This injects the DNS resolver into transport constructors.

@@ -28,43 +29,47 @@ var (
connGaterType = reflect.TypeOf((*connmgr.ConnectionGater)(nil)).Elem()
upgraderType = reflect.TypeOf((*transport.Upgrader)(nil)).Elem()
rcmgrType = reflect.TypeOf((*network.ResourceManager)(nil)).Elem()
resolverType = reflect.TypeOf((*madns.Resolver)(nil)).Elem()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to use an interface instead of a concrete type here. madns.BasicResolver?

Could it be that madns.Resolver and madns.BasicResolver should switch their names?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, madns.Resolver is basically a light-weight wrapper over a set of BasicResolvers that implements a multiaddr handing Resolve method. The pluggable component is really madns.BasicResolver, where the user should then construct an madns.Resolver from one or more madns.BasicResolvers.

@BigLep
Copy link
Contributor

BigLep commented Aug 19, 2022

2022-08-19: this is needed for 0.23 to fix address resolution issues. This is related to #1597 .

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.

@marten-seemann
Copy link
Contributor

Rebased to resolve merge conflict in imports.

@marten-seemann marten-seemann merged commit 0efe1e5 into master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants