[improve][client] Add a way to configure which DNS use by diegosalvi · Pull Request #21227 · apache/pulsar

diegosalvi · 2023-09-21T13:47:12Z

This closes #21226

Motivation

Currently is not possibile to configure one or more dns servers that will be used by the client to resolve hostnames.

Modifications

Added an optional configuration ClientConfigurationData.dnsServerAddresses to allow to configure custom DNSs. It not provided or empty will default to the existing behaviour.
Handled the new conf in ConnectionPool.

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

Added unit test on ClientBuilderImplTest with configuration of both good or wrong dns servers
Added integration test on PulsarClientImplTest with a custom configured dns servers list
Added unit test on ConfigurationDataUtilsTest about the new configuration loading

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

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

nodece

Reviewed

diegosalvi · 2023-09-27T13:55:50Z

Rebased on synced fork

diegosalvi · 2023-10-02T06:50:32Z

Last review issue has been addressed

eolivelli

Lgtm

nodece · 2023-10-03T06:17:33Z

/pulsarbot rerun-failure-checks

diegosalvi · 2023-10-04T14:23:48Z

Fails org.apache.pulsar.common.nar.NarUnpackerTest.shouldExtractFilesOnceInDifferentProcess
Could it be flaky?

nodece · 2023-10-05T04:16:05Z

@diegosalvi Could you rebase your codebase?

lhotari · 2023-10-05T10:59:56Z

@diegosalvi Could you please provide more context of the use case? The reason why I'm asking is that I don't currently see why the Pulsar client should have a different DNS server configured apart from the rest of the application and the OS where the app is running.

lhotari · 2023-10-05T11:01:44Z

Fails org.apache.pulsar.common.nar.NarUnpackerTest.shouldExtractFilesOnceInDifferentProcess Could it be flaky?

Yes, it's flaky. reported as #21291

diegosalvi · 2023-10-05T16:02:59Z

@lhotari currently we are using Pulsar in a production environment. In such environment we have need to use different dns for the server machine (configured by standard resolvconf et simillia) and java processes. For many java cases we directly do lookups on dns with different libraries (so we don't use java discovered dnses) but we can't do the same for Pulsar.
To be able to configure which dns use can be useful depending on environments to specialize them

diegosalvi · 2023-10-05T16:06:08Z

@nodece Rebased

lhotari · 2023-10-05T16:37:32Z

@lhotari currently we are using Pulsar in a production environment. In such environment we have need to use different dns for the server machine (configured by standard resolvconf et simillia) and java processes. For many java cases we directly do lookups on dns with different libraries (so we don't use java discovered dnses) but we can't do the same for Pulsar.
To be able to configure which dns use can be useful depending on environments to specialize them

@diegosalvi for future documentation purposes, would you be able to share more details and a common reference? Is it a split DNS setup where you have an internal DNS separate from an external DNS ? Would you configure the Pulsar client to use the internal DNS server?

diegosalvi · 2023-10-06T12:49:03Z

@lhotari Yes, something along the line. DNS for Pulsar are "internal" "applicative" ones leaving the host machine with a different set of dns (via resolv.conf etc etc) and other "external" DNS queried directly by java processes when in need to resolve "public" names.

diegosalvi · 2023-10-09T11:58:03Z

Still fails for NarUnpackerTest

lhotari · 2023-10-09T13:52:37Z

Still fails for NarUnpackerTest

@diegosalvi This flaky NarUnpackerTest has been fixed. You can get the fix to this PR by rebasing on or merging origin/master.

diegosalvi · 2023-10-09T14:04:28Z

Rebased again

codecov-commenter · 2023-10-10T02:18:30Z

Codecov Report

Merging #21227 (53f5241) into master (8438e43) will increase coverage by 36.37%.
Report is 6 commits behind head on master.
The diff coverage is 80.64%.

@@              Coverage Diff              @@
##             master   #21227       +/-   ##
=============================================
+ Coverage     36.84%   73.22%   +36.37%     
- Complexity    12229    32529    +20300     
=============================================
  Files          1699     1888      +189     
  Lines        130559   140257     +9698     
  Branches      14264    15445     +1181     
=============================================
+ Hits          48106   102703    +54597     
+ Misses        76121    29457    -46664     
- Partials       6332     8097     +1765

Flag	Coverage Δ
inttests	`24.19% <45.16%> (-0.14%)`	⬇️
systests	`24.73% <41.93%> (+<0.01%)`	⬆️
unittests	`72.53% <80.64%> (+40.58%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java	`71.76% <100.00%> (+30.24%)`	⬆️
...n/java/org/apache/pulsar/broker/service/Topic.java	`36.36% <ø> (+28.03%)`	⬆️
...lsar/client/impl/conf/ClientConfigurationData.java	`96.69% <100.00%> (+8.45%)`	⬆️
...g/apache/pulsar/client/impl/ClientBuilderImpl.java	`85.98% <83.33%> (+39.62%)`	⬆️
.../org/apache/pulsar/client/impl/ConnectionPool.java	`75.96% <66.66%> (+14.01%)`	⬆️
...ava/org/apache/pulsar/broker/service/Producer.java	`82.64% <75.00%> (+20.21%)`	⬆️

... and 1444 files with indirect coverage changes

nodece

This PR adds a public method to ClientBuilder.java, I'm not sure if PIP is required.

LGTM

tisonkun

I agree with this proposal and since it adds a new public API, I suppose a PIP is required to create, while it should be very straight forward and we can quickly reach a consensus.

@diegosalvi what do you think? You can refer to this page for sending a PIP.

cc @codelipenghui @Technoboy-

tisonkun · 2023-10-24T18:58:30Z

Pending for merging as PIP-305 #21352 merged.

tisonkun

Thank you!

github-actions Bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 21, 2023

Technoboy- assigned diegosalvi Sep 25, 2023

nodece reviewed Sep 25, 2023

View reviewed changes

Comment thread pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientBuilderImplTest.java

nodece reviewed Sep 25, 2023

View reviewed changes

Comment thread pulsar-client/src/test/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtilsTest.java

nodece requested changes Sep 25, 2023

View reviewed changes

diegosalvi requested a review from nodece September 26, 2023 08:31

nodece reviewed Sep 26, 2023

View reviewed changes

Comment thread pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientBuilderImplTest.java Outdated

diegosalvi requested a review from nodece September 26, 2023 12:53

diegosalvi force-pushed the client-builder-dns-servers branch from b8d2756 to 48b6c0c Compare September 27, 2023 13:54

eolivelli approved these changes Oct 2, 2023

View reviewed changes

eolivelli added the ready-to-test label Oct 2, 2023

diegosalvi force-pushed the client-builder-dns-servers branch from 1b57ce5 to 9e2bc1a Compare October 5, 2023 16:05

diegosalvi added 3 commits October 9, 2023 16:02

Allow Client Builder set Dns server param

cf1bc6d

Fix review notes

bcd919e

More test fixes

53f5241

diegosalvi force-pushed the client-builder-dns-servers branch from 9e2bc1a to 53f5241 Compare October 9, 2023 14:03

nodece approved these changes Oct 10, 2023

View reviewed changes

tisonkun reviewed Oct 10, 2023

View reviewed changes

Merge branch 'apache:master' into client-builder-dns-servers

6bac8c4

diegosalvi mentioned this pull request Oct 12, 2023

[improve][pip] PIP-305: Customize DNS servers to use for Pulsar Client #21352

Merged

14 tasks

Merge branch 'master' into client-builder-dns-servers

2130dce

tisonkun approved these changes Oct 24, 2023

View reviewed changes

tisonkun merged commit 25c662d into apache:master Oct 24, 2023

Conversation

diegosalvi commented Sep 21, 2023 • edited by tisonkun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

Uh oh!

nodece left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

diegosalvi commented Sep 27, 2023

Uh oh!

diegosalvi commented Oct 2, 2023

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

nodece commented Oct 3, 2023

Uh oh!

diegosalvi commented Oct 4, 2023

Uh oh!

nodece commented Oct 5, 2023

Uh oh!

lhotari commented Oct 5, 2023

Uh oh!

lhotari commented Oct 5, 2023

Uh oh!

diegosalvi commented Oct 5, 2023

Uh oh!

diegosalvi commented Oct 5, 2023

Uh oh!

lhotari commented Oct 5, 2023

Uh oh!

diegosalvi commented Oct 6, 2023

Uh oh!

diegosalvi commented Oct 9, 2023

Uh oh!

lhotari commented Oct 9, 2023

Uh oh!

diegosalvi commented Oct 9, 2023

Uh oh!

codecov-commenter commented Oct 10, 2023

Codecov Report

Uh oh!

nodece left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Oct 24, 2023

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

diegosalvi commented Sep 21, 2023 •

edited by tisonkun

Loading

tisonkun left a comment •

edited

Loading