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 configurable DNS client for ConnectionPool #6382

Closed

Conversation

diegosalvi
Copy link
Contributor

Fixes #6346

Motivation

Currently DNS client for PulsarClient cannot be configured and we are struck with default one. Specifically I needed to be able to configure the dns server list

Modifications

  • Added a new method: ClientBuilder.dns
  • Used configured dns builder instead of creating a new default one if provided in ConnectionPool

Verifying this change

  • Make sure that the change passes the CI checks.

This change is essentially a trivial rework, it just expose the ability to provider a custom DnsNameResolverBuilder and use it if provided.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no) yes (added dependency on netty-dns-resolver to client api)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no) yes
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) JavaDocs
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@diegosalvi the change looks good. although there is one small problem we need to think about - the pulsar-client is shipped by shading its dependencies. Hence DnsNameResolverBuilder will be shaded as well. so the end-users will be using the shaded class for configuring the DnsResolver.

So I would suggest defining a DnsResolverConfig in pulsar-client-api, and allow use configuring a DnsResolver by passing in a DnsResolverConfig. So we don't directly expose netty classes in Pulsar API.

@sijie sijie added this to the 2.6.0 milestone Feb 22, 2020
@sijie sijie added the type/feature The PR added a new feature or issue requested a new feature label Feb 22, 2020
@diegosalvi
Copy link
Contributor Author

I'll add a configuration bean then :)

@codelipenghui
Copy link
Contributor

@diegosalvi Any updates on this PR? Move this PR to 2.7.0 and If there is any update, move it back.

@codelipenghui codelipenghui modified the milestones: 2.6.0, 2.7.0 Jun 2, 2020
@codelipenghui codelipenghui modified the milestones: 2.7.0, 2.8.0 Nov 4, 2020
@codelipenghui codelipenghui removed this from the 2.8.0 milestone May 19, 2021
@benlongo
Copy link

Any updates on the state of this?

@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 15, 2022
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@tisonkun
Copy link
Member

tisonkun commented Dec 6, 2022

We support part of DNS configuration in #15219 by @lhotari.

To allow passing a DnsNameResolverBuilder in ClientBuilder, it can be a significant public interface change and I'd suggest go through a PIP process.

Closed as stale and conflict.

@tisonkun tisonkun closed this Dec 6, 2022
@tisonkun tisonkun mentioned this pull request Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS configuration
5 participants