Skip to content

Add support to specify pre-resolved IP addresses and avoid additional DNS lookup #2

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

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

rjobanp
Copy link

@rjobanp rjobanp commented Mar 25, 2024

Necessary to support this PR MaterializeInc/materialize#26186 by allowing us to specify multiple pre-resolved IP addresses to avoid a DNS lookup inside the library.

I wanted to know what you thought of the API changes before opening the upstream PR. Since we recently also added support for an explicit TLS hostname override https://docs.rs/mysql_async/latest/mysql_async/struct.SslOpts.html#method.with_danger_tls_hostname_override it seems a bit clunky to keep the hostname field on the new HostPortOrUrl::ResolvedHost enum type I introduced, but without that field it's unclear what we should return from get_ip_or_hostname, which is behind a public method returning a &str

@rjobanp rjobanp requested review from benesch and petrosagg March 25, 2024 17:57
@benesch
Copy link

benesch commented Mar 25, 2024

I wanted to know what you thought of the API changes before opening the upstream PR.

I feel like we should be able to do a big unification/simplification? It seems like we could remove with_danger_tls_hostname_override, in favor of the new resolved_ips API?

@rjobanp
Copy link
Author

rjobanp commented Mar 25, 2024

I feel like we should be able to do a big unification/simplification? It seems like we could remove with_danger_tls_hostname_override, in favor of the new resolved_ips API?

I'm not sure if the ergonomics would be the same. If someone still wanted this library to resolve DNS and to override the TLS hostname, there wouldn't be a way to do that with the resolved_ips API. So I think we still need both, but I will incorporate your other comments to avoid introducing the 3rd enum variant of HostPortOrUrl

@benesch
Copy link

benesch commented Mar 25, 2024

If someone still wanted this library to resolve DNS and to override the TLS hostname, there wouldn't be a way to do that with the resolved_ips API.

Yes, agreed, it would somewhat limit the ergonomics. Arguably fine since before last month no one had wanted the with_danger_tls_hostname_override API, and as of this month we don't need it either. I don't feel strongly though, and happy to defer to you/upstream on whatever is most palatable.

@rjobanp
Copy link
Author

rjobanp commented Mar 25, 2024

Sounds good. I'll defer to upstream when I open a PR for this branch there and ask if it would make sense to remove the with_danger_tls_hostname_override API.

Copy link

@benesch benesch left a comment

Choose a reason for hiding this comment

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

This looks great to me!

@rjobanp rjobanp merged commit 6afd213 into master Mar 25, 2024
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.

2 participants