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 custom dns resolver to BlazeClientBuilder. #4699

Merged

Conversation

JakDar
Copy link
Contributor

@JakDar JakDar commented Apr 3, 2021

Hi, AFAIK there is currently no way to add custom DNS resolver for blaze-client (similar to one provided in akka-http (this PR and here in docs).
I kinda need that feature, potential use cases are in linked akkka-http doc.

I added new parameter in BlazeClientBuilder - customDnsResolver - which is passed to Http1Support and if present - used instead of getAddress.

I'm glad to adjust/change implementation if it is not suitable.

Also, are changes from series/0.21 eventually merged to 1.0.0 series (main), or should I submit separate PR for it?

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Thanks! Yes, this is eventually merged to series/0.22 and main, and would be in all three releases. Starting it here lets you use it in a stable release ASAP.

Binary compatibility is broken because the constructor for BlazeClientBuilder changed. We can fix that with a deprecated auxiliary constructor (example, or I guess just add a MiMa exception since it's private. I usually prefer the deprecated constructors, just to be 100% sure.

This might be a compelling feature in Ember, too, but we can handle that in a separate PR.

@@ -57,7 +58,8 @@ sealed abstract class BlazeClientBuilder[F[_]] private (
val executionContext: ExecutionContext,
val scheduler: Resource[F, TickWheelExecutor],
val asynchronousChannelGroup: Option[AsynchronousChannelGroup],
val channelOptions: ChannelOptions
val channelOptions: ChannelOptions,
val customDnsResolver: Option[RequestKey => Either[Throwable, InetSocketAddress]]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this needs to be an Option: could we just have a default based on the getAddress that's used today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also moved getAddress to BlazeClientBuilder.

@@ -57,7 +58,8 @@ sealed abstract class BlazeClientBuilder[F[_]] private (
val executionContext: ExecutionContext,
val scheduler: Resource[F, TickWheelExecutor],
val asynchronousChannelGroup: Option[AsynchronousChannelGroup],
val channelOptions: ChannelOptions
val channelOptions: ChannelOptions,
val customDnsResolver: Option[RequestKey => Either[Throwable, InetSocketAddress]]
Copy link
Member

Choose a reason for hiding this comment

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

Something for 1.0: we'll probably want to refine this to the ip4s address type once it becomes public API.

Well, maybe not, if blaze is just using java.net internally. It's a bigger issue for things with deeper fs2 integration. Anyway, something to think about.

@rossabaker rossabaker added the enhancement Feature requests and improvements label Apr 3, 2021
@JakDar JakDar requested a review from rossabaker April 3, 2021 16:38
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Thanks! I expect a release train for Scala 3.0.0-RC2 support in the next few days, and we'll include 0.21 as well to pick this up.

@rossabaker rossabaker merged commit b34f7f4 into http4s:series/0.21 Apr 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants