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

[fix][client] Prevent DNS reverse lookup when physical address is an IP address #19028

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Dec 22, 2022

Motivation

A call to java.net.InetSocketAddress#getHostName will trigger a reverse DNS lookup when the physical address is an IP address. This is not desired since the DNS resolver will be blocking execution.

Modifications

Use java.net.InetSocketAddress#getHostString to use the address passed as input (host string or ip address).

Documentation

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

Matching PR in forked repository

PR in forked repository: lhotari#120

@lhotari lhotari added this to the 2.12.0 milestone Dec 22, 2022
@lhotari lhotari self-assigned this Dec 22, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 22, 2022
@Technoboy- Technoboy- closed this Dec 23, 2022
@Technoboy- Technoboy- reopened this Dec 23, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Merging #19028 (6874f65) into master (08591d9) will decrease coverage by 4.49%.
The diff coverage is 11.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19028      +/-   ##
============================================
- Coverage     49.85%   45.35%   -4.50%     
- Complexity     8658    10898    +2240     
============================================
  Files           500      769     +269     
  Lines         54930    74180   +19250     
  Branches       5867     7981    +2114     
============================================
+ Hits          27386    33648    +6262     
- Misses        24464    36764   +12300     
- Partials       3080     3768     +688     
Flag Coverage Δ
unittests 45.35% <11.57%> (-4.50%) ⬇️

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

Impacted Files Coverage Δ
...g/apache/pulsar/broker/admin/impl/TenantsBase.java 96.45% <ø> (ø)
...he/pulsar/broker/admin/v2/NonPersistentTopics.java 62.03% <ø> (ø)
...pache/pulsar/broker/admin/v2/PersistentTopics.java 74.25% <ø> (+2.56%) ⬆️
...rg/apache/pulsar/broker/delayed/bucket/Bucket.java 0.00% <0.00%> (ø)
...r/delayed/bucket/BucketDelayedDeliveryTracker.java 0.00% <0.00%> (ø)
.../pulsar/broker/delayed/bucket/ImmutableBucket.java 0.00% <0.00%> (ø)
...g/apache/pulsar/broker/lookup/TopicLookupBase.java 55.17% <ø> (+4.33%) ⬆️
...rg/apache/pulsar/broker/web/PulsarWebResource.java 57.41% <ø> (+1.75%) ⬆️
.../org/apache/pulsar/client/impl/ConnectionPool.java 37.43% <0.00%> (ø)
...va/org/apache/pulsar/broker/service/ServerCnx.java 49.41% <33.33%> (+1.59%) ⬆️
... and 316 more

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit d8569cd into apache:master Dec 23, 2022
michaeljmarshall pushed a commit that referenced this pull request Jan 31, 2023
michaeljmarshall pushed a commit that referenced this pull request Jan 31, 2023
michaeljmarshall pushed a commit that referenced this pull request Jan 31, 2023
@michaeljmarshall
Copy link
Member

I cherry picked this change to older branches because it was going to be introduced by #19327.

michaeljmarshall pushed a commit that referenced this pull request Jan 31, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…IP address (apache#19028)

(cherry picked from commit d8569cd)
(cherry picked from commit 5385f3b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants