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

Fixes hostname retrieval triggering possible DNS lookups for Thrift plugin #4427

Merged
merged 1 commit into from Aug 1, 2018

Conversation

Xylus
Copy link
Contributor

@Xylus Xylus commented Jul 31, 2018

This solves the issue where the Thrift plugin would attempt a reverse DNS lookup when retrieving the host name if a TSocket was created using an address.
Utility methods are also added, which given an InetSocketAddress, would retrieve the host name or the address without triggering forward/reverse DNS lookup.

resolves #4389

@Xylus Xylus requested a review from koo-taejin July 31, 2018 09:22
@codecov-io
Copy link

codecov-io commented Jul 31, 2018

Codecov Report

Merging #4427 into master will increase coverage by 0.04%.
The diff coverage is 26.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4427      +/-   ##
==========================================
+ Coverage   40.78%   40.83%   +0.04%     
==========================================
  Files        2487     2488       +1     
  Lines       75672    75687      +15     
  Branches    10243    10238       -5     
==========================================
+ Hits        30863    30904      +41     
+ Misses      41975    41952      -23     
+ Partials     2834     2831       -3
Impacted Files Coverage Δ
...eptor/server/TBaseProcessorProcessInterceptor.java 0% <ø> (ø) ⬆️
.../navercorp/pinpoint/plugin/thrift/ThriftUtils.java 0% <0%> (ø) ⬆️
...col/server/TProtocolReadMessageEndInterceptor.java 0% <0%> (ø) ⬆️
.../com/navercorp/pinpoint/profiler/DefaultAgent.java 0% <0%> (ø) ⬆️
...oint/bootstrap/plugin/util/SocketAddressUtils.java 42.5% <42.5%> (ø)
...vercorp/pinpoint/web/vo/ApplicationAgentsList.java 72.3% <0%> (-3.08%) ⬇️
...tor/metric/datasource/DefaultDataSourceMetric.java 75.75% <0%> (-3.04%) ⬇️
...cluster/zookeeper/ZookeeperClusterDataManager.java 64.62% <0%> (-0.69%) ⬇️
...point/rpc/client/DefaultPinpointClientHandler.java 71.65% <0%> (+1.27%) ⬆️
... and 7 more

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 6e697f4...e859672. Read the comment docs.

private static HostStringAccessor createHostStringAccessor() {
try {
final Method m = InetSocketAddress.class.getDeclaredMethod("getHostString");
m.setAccessible(true);
Copy link
Member

Choose a reason for hiding this comment

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

getHostString() in jdk6 is a package level accessor.
So all versions of jdk1.6 should be checked that method is included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koo-taejin Thanks for the reminder.
getHostString() is present in 1.6.0_0 and in 1.6.0_45, so it should be there for other versions of 1.6 as well.

Copy link
Member

@koo-taejin koo-taejin left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@Xylus Xylus merged commit 10905af into pinpoint-apm:master Aug 1, 2018
@Xylus Xylus deleted the bugfix/issue-4389 branch August 1, 2018 07:32
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.

thrift plugin InetSocketAddress#getHostName issue
3 participants