[SPARK-22030][CORE] GraphiteSink fails to re-connect to Graphite instances behind an ELB or any other auto-scaled LB #19210
Conversation
…resolve hosts behind a CNAME with re-tried DNS lookups Upgrade codahale metrics library so that Graphite constructor can re-resolve hosts behind a CNAME with re-tried DNS lookups. When Graphite is deployed behind an ELB, ELB may change IP addresses based on auto-scaling needs. Using current approach yields Graphite usage impossible, fixing for that use case
… behind auto-scaled load balancers This fix is introduced in codahale 3.1.5 - dropwizard/metrics@v3.1.2...v3.1.5#diff-6916c85d2dd08d89fe771c952e3b8512R120
alexmnyc
changed the title
|
You should also update the the files under |
|
@jerryshao thanks. it's done. |
|
@HyukjinKwon would you please help to trigger the Jenkins? Thanks! |
|
Sure. ok to test |
|
ok to test |
|
Test build #81754 has finished for PR 19210 at commit
|
|
Jenkins, retest this please. |
|
Test build #81770 has finished for PR 19210 at commit
|
| val graphite = propertyToOption(GRAPHITE_KEY_PROTOCOL).map(_.toLowerCase(Locale.ROOT)) match { | ||
| case Some("udp") => new GraphiteUDP(new InetSocketAddress(host, port)) | ||
| case Some("tcp") | None => new Graphite(new InetSocketAddress(host, port)) | ||
| case Some("tcp") | None => new Graphite(host, port) |
There was a problem hiding this comment.
Do we need to change here also?
There was a problem hiding this comment.
@jerryshao Do you mean change Graphite constructor for "udp" case ?
There was a problem hiding this comment.
Yes, that's what I mean, I'm not sure if it is required since I'm not familiar with this Graphite sink.
There was a problem hiding this comment.
Looks like there is a constructor accepting hostname directly for UDP. I'll add that as well.
|
@jerryshao done |
|
BTW, can you please create a JIRA, and fix the PR title like other PRs. |
|
Test build #81802 has finished for PR 19210 at commit
|
|
@jerryshao No problem. I don't have a JIRA account for Apache though. How do I go about getting one? - Actually NM, I found it. Will do. Thanks, Jerry. |
alexmnyc
changed the title
alexmnyc
changed the title
|
@jerryshao done |
|
Jenkins, retest this please. |
|
LGTM, let me retest this again. |
|
Test build #81875 has finished for PR 19210 at commit
|
|
LGTM, merging to master. |
4 participants
What changes were proposed in this pull request?
Upgrade codahale metrics library so that Graphite constructor can re-resolve hosts behind a CNAME with re-tried DNS lookups. When Graphite is deployed behind an ELB, ELB may change IP addresses based on auto-scaling needs. Using current approach yields Graphite usage impossible, fixing for that use case
How was this patch tested?
The same logic is used for another project that is using the same configuration and code path, and graphite re-connect's behind ELB's are no longer an issue
This are proposed changes for codahale lib - dropwizard/metrics@v3.1.2...v3.1.5#diff-6916c85d2dd08d89fe771c952e3b8512R120. Specifically, https://github.com/dropwizard/metrics/blob/b4d246d34e8a059b047567848b3522567cbe6108/metrics-graphite/src/main/java/com/codahale/metrics/graphite/Graphite.java#L120
Please review http://spark.apache.org/contributing.html before opening a pull request.