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

imlement issue #480 : UDP target host address is cached #502

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

lxhoan
Copy link
Contributor

@lxhoan lxhoan commented Aug 29, 2018

this PR is a simpler solution for #480

@lxhoan
Copy link
Contributor Author

lxhoan commented Aug 29, 2018

Hi @fmachado , @majst01 , in case we have dns resolving cached at JVM level, still need this PR to fix issue #480

@codecov-io
Copy link

codecov-io commented Aug 29, 2018

Codecov Report

Merging #502 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #502      +/-   ##
============================================
- Coverage     87.53%   87.45%   -0.09%     
+ Complexity      365      364       -1     
============================================
  Files            25       25              
  Lines          1500     1506       +6     
  Branches        167      167              
============================================
+ Hits           1313     1317       +4     
- Misses          120      123       +3     
+ Partials         67       66       -1
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/impl/InfluxDBImpl.java 89.97% <100%> (+0.16%) 78 <2> (ø) ⬇️
src/main/java/org/influxdb/InfluxDBException.java 87.27% <0%> (-3.64%) 11% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24c5542...9434330. Read the comment docs.

@lxhoan lxhoan added this to the 2.13 milestone Aug 29, 2018
@lxhoan
Copy link
Contributor Author

lxhoan commented Aug 30, 2018

Hi @majst01 , @fmachado
What do you think ?
tks

@majst01
Copy link
Collaborator

majst01 commented Aug 30, 2018

Hi @lxhoan

I like this approach, but we should at least measure what performance impact this gives ?

Copy link
Contributor

@fmachado fmachado left a comment

Choose a reason for hiding this comment

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

Except for my last comment, I'm fine with merging this PR.

private InetAddress parseHostAddress(final String url) {
HttpUrl httpUrl = HttpUrl.parse(url);
private String parseHost(final String url) {
HttpUrl httpUrl = HttpUrl.parse(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is HttpUrl able to parse URIs that do not start with the string http? IMO we should treat this parameter as an URI and not an URL. The main reasons are:

  • Users may finally stop using the scheme "http://" to connect via UDP. RFC 1738 for URL does not allow the scheme "udp://" but the RFC 3986 for URI does.
  • If so, this line could be replaced with URI.create(...) and later you could use URI#getHost() to get only the hostname.

@lxhoan
Copy link
Contributor Author

lxhoan commented Sep 5, 2018

@fmachado , I have fixed as your comment, tks

@lxhoan
Copy link
Contributor Author

lxhoan commented Sep 5, 2018

Hi @majst01 ,

I like this approach, but we should at least measure what performance impact this gives ?

Performance impact should be inconsiderable since DNS resolving is cached at JVM level (as we discussed intensively before), so most of InetSocketAddress resolving is are just intra-process invocations

@lxhoan lxhoan closed this Sep 5, 2018
@lxhoan lxhoan reopened this Sep 5, 2018
@lxhoan lxhoan removed this from the 2.13 milestone Sep 5, 2018
@majst01 majst01 merged commit bebe7c3 into influxdata:master Sep 6, 2018
@lxhoan lxhoan added this to the 2.13 milestone Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants