You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "diegosalvi (via GitHub)" <gi...@apache.org> on 2023/09/21 13:47:14 UTC

[GitHub] [pulsar] diegosalvi opened a new pull request, #21227: [improve][client] Add a way to configure which DNS use

diegosalvi opened a new pull request, #21227:
URL: https://github.com/apache/pulsar/pull/21227

   Main issue #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:
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   *If the box was checked, please highlight the changes*
   
   - [ ] Dependencies (add or upgrade a dependency)
   - [ ] The public API
   - [ ] The schema
   - [ ] The default values of configurations
   - [ ] The threading model
   - [ ] The binary protocol
   - [ ] The REST endpoints
   - [ ] The admin CLI options
   - [ ] The metrics
   - [ ] Anything that affects deployment
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: <!-- ENTER URL HERE -->
   
   <!--
   After opening this PR, the build in apache/pulsar will fail and instructions will
   be provided for opening a PR in the PR author's forked repository.
   
   apache/pulsar pull requests should be first tested in your own fork since the 
   apache/pulsar CI based on GitHub Actions has constrained resources and quota.
   GitHub Actions provides separate quota for pull requests that are executed in 
   a forked repository.
   
   The tests will be run in the forked repository until all PR review comments have
   been handled, the tests pass and the PR is approved by a reviewer.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1746985086

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1753079548

   Rebased again


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1744278713

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] diegosalvi commented on a diff in pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on code in PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#discussion_r1336831431


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientBuilderImplTest.java:
##########
@@ -101,6 +103,17 @@ public void testClientBuilderWithIllegalLargePort() throws PulsarClientException
         PulsarClient.builder().dnsLookupBind("localhost", 65536).build();
     }
 
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testClientBuilderWithIllegalDNSServerHostname() throws PulsarClientException {
+        PulsarClient.builder().dnsServerAddresses(
+                Arrays.asList(new InetSocketAddress("1.2.3.4", 53), new InetSocketAddress("localhost",53))).build();
+    }
+
+    public void testClientBuilderWithDNSServerIP() throws PulsarClientException {

Review Comment:
   Added



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] diegosalvi commented on a diff in pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on code in PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#discussion_r1336832270


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtilsTest.java:
##########
@@ -61,8 +63,10 @@ public void testLoadClientConfigurationData() {
         config.put("authParams", "testAuthParams");
         config.put("authParamMap", authParamMap);
         config.put("dnsLookupBindAddress", "0.0.0.0");
-        config.put("dnsLookupBindPort", 0);

Review Comment:
   Good catch! It was totally a wrong "cleanup"!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on a diff in pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#discussion_r1336040773


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientBuilderImplTest.java:
##########
@@ -101,6 +103,17 @@ public void testClientBuilderWithIllegalLargePort() throws PulsarClientException
         PulsarClient.builder().dnsLookupBind("localhost", 65536).build();
     }
 
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testClientBuilderWithIllegalDNSServerHostname() throws PulsarClientException {
+        PulsarClient.builder().dnsServerAddresses(
+                Arrays.asList(new InetSocketAddress("1.2.3.4", 53), new InetSocketAddress("localhost",53))).build();
+    }
+
+    public void testClientBuilderWithDNSServerIP() throws PulsarClientException {

Review Comment:
   Miss `@Test`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1750617006

   @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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #21227:
URL: https://github.com/apache/pulsar/pull/21227


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1749281860

   > @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? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on a diff in pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#discussion_r1336995071


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientBuilderImplTest.java:
##########
@@ -101,6 +103,18 @@ public void testClientBuilderWithIllegalLargePort() throws PulsarClientException
         PulsarClient.builder().dnsLookupBind("localhost", 65536).build();
     }
 
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testClientBuilderWithIllegalDNSServerHostname() throws PulsarClientException {
+        PulsarClient.builder().dnsServerAddresses(
+                Arrays.asList(new InetSocketAddress("1.2.3.4", 53), new InetSocketAddress("localhost",53))).build();
+    }
+
+    @Test()
+    public void testClientBuilderWithDNSServerIP() throws PulsarClientException {
+        PulsarClient.builder().dnsServerAddresses(
+                Arrays.asList(new InetSocketAddress("1.2.3.4", 53))).build();

Review Comment:
   Could you remove `build()`? Because you don't setup the broker URL.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1753058515

   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1748662261

   > Fails org.apache.pulsar.common.nar.NarUnpackerTest.shouldExtractFilesOnceInDifferentProcess Could it be flaky?
   
   Yes, it's flaky. reported as #21291


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1777845704

   Pending for merging as PIP-305 #21352 merged.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1752873169

   Still fails for NarUnpackerTest


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1748006927

   @diegosalvi Could you rebase your codebase?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] diegosalvi commented on pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1742483013

   Last review issue has been addressed


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on a diff in pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#discussion_r1336041924


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtilsTest.java:
##########
@@ -61,8 +63,10 @@ public void testLoadClientConfigurationData() {
         config.put("authParams", "testAuthParams");
         config.put("authParamMap", authParamMap);
         config.put("dnsLookupBindAddress", "0.0.0.0");
-        config.put("dnsLookupBindPort", 0);

Review Comment:
   Why not remove `config.put("dnsLookupBindPort", 0);`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] diegosalvi commented on pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1737453163

   Rebased on synced fork


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1749199051

   @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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1749204140

   @nodece Rebased


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1748659807

   @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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][client] Add a way to configure which DNS use [pulsar]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#issuecomment-1754218903

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#21227](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (53f5241) into [master](https://app.codecov.io/gh/apache/pulsar/commit/8438e43fb02b6d8df29672c6460a2f3da924ab60?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8438e43) will **increase** coverage by `36.37%`.
   > Report is 6 commits behind head on master.
   > The diff coverage is `80.64%`.
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/21227/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@              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](https://app.codecov.io/gh/apache/pulsar/pull/21227/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21227/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.19% <45.16%> (-0.14%)` | :arrow_down: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/21227/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.73% <41.93%> (+<0.01%)` | :arrow_up: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21227/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.53% <80.64%> (+40.58%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...va/org/apache/pulsar/broker/service/ServerCnx.java](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1NlcnZlckNueC5qYXZh) | `71.76% <100.00%> (+30.24%)` | :arrow_up: |
   | [...n/java/org/apache/pulsar/broker/service/Topic.java](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1RvcGljLmphdmE=) | `36.36% <ø> (+28.03%)` | :arrow_up: |
   | [...lsar/client/impl/conf/ClientConfigurationData.java](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL2NvbmYvQ2xpZW50Q29uZmlndXJhdGlvbkRhdGEuamF2YQ==) | `96.69% <100.00%> (+8.45%)` | :arrow_up: |
   | [...g/apache/pulsar/client/impl/ClientBuilderImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NsaWVudEJ1aWxkZXJJbXBsLmphdmE=) | `85.98% <83.33%> (+39.62%)` | :arrow_up: |
   | [.../org/apache/pulsar/client/impl/ConnectionPool.java](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25Qb29sLmphdmE=) | `75.96% <66.66%> (+14.01%)` | :arrow_up: |
   | [...ava/org/apache/pulsar/broker/service/Producer.java](https://app.codecov.io/gh/apache/pulsar/pull/21227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1Byb2R1Y2VyLmphdmE=) | `82.64% <75.00%> (+20.21%)` | :arrow_up: |
   
   ... and [1444 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21227/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] diegosalvi commented on a diff in pull request #21227: [improve][client] Add a way to configure which DNS use

Posted by "diegosalvi (via GitHub)" <gi...@apache.org>.
diegosalvi commented on code in PR #21227:
URL: https://github.com/apache/pulsar/pull/21227#discussion_r1337161650


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientBuilderImplTest.java:
##########
@@ -101,6 +103,18 @@ public void testClientBuilderWithIllegalLargePort() throws PulsarClientException
         PulsarClient.builder().dnsLookupBind("localhost", 65536).build();
     }
 
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testClientBuilderWithIllegalDNSServerHostname() throws PulsarClientException {
+        PulsarClient.builder().dnsServerAddresses(
+                Arrays.asList(new InetSocketAddress("1.2.3.4", 53), new InetSocketAddress("localhost",53))).build();
+    }
+
+    @Test()
+    public void testClientBuilderWithDNSServerIP() throws PulsarClientException {
+        PulsarClient.builder().dnsServerAddresses(
+                Arrays.asList(new InetSocketAddress("1.2.3.4", 53))).build();

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org