Update Eureka Server's service-urls upon EnvironmentChangeEvent.

[fahimfarookme]

Fixes gh-2421

[codecov-io]

Codecov Report

Merging #2455 into master will increase coverage by 2.64%.
The diff coverage is 88.88%.

@@            Coverage Diff            @@
##           master   #2455      +/-   ##
=========================================
+ Coverage   65.65%   68.3%   +2.64%     
=========================================
  Files         207     236      +29     
  Lines        7644    8324     +680     
  Branches      943    1039      +96     
=========================================
+ Hits         5019    5686     +667     
+ Misses       2267    2248      -19     
- Partials      358     390      +32

Impacted Files	Coverage Δ
...x/eureka/server/EurekaServerAutoConfiguration.java	91.89% <88.88%> (-1.22%)	⬇️
...rk/cloud/netflix/eureka/EurekaDiscoveryClient.java	54.76% <0%> (-10.96%)	⬇️
...ix/hystrix/HystrixCircuitBreakerConfiguration.java	89.83% <0%> (-10.17%)	⬇️
...ork/cloud/netflix/feign/FeignClientProperties.java	62.26% <0%> (-3.78%)	⬇️
.../cloud/netflix/eureka/server/EurekaController.java	67.27% <0%> (-1.22%)	⬇️
...k/cloud/netflix/eureka/EurekaClientConfigBean.java	28.31% <0%> (-0.7%)	⬇️
.../netflix/eureka/EurekaClientAutoConfiguration.java	91.3% <0%> (ø)	⬆️
...ork/cloud/netflix/feign/support/SpringEncoder.java	65.11% <0%> (ø)	⬆️
...ework/cloud/netflix/sidecar/SidecarProperties.java	41.93% <0%> (ø)	⬆️
... and 175 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 4e8d588...e2dcf30. Read the comment docs.

[spencergibb]

I would only update if specific keys are changed

[fahimfarookme]

Thanks @ryanjbaxter @spencergibb.

Yeah, that makes sense and hope change to either availability-zone,region or service-urls should trigger the update.

[spencergibb]

Great, we look forward to that update.

[fahimfarookme]

@spencergibb @ryanjbaxter Checked-in and rebased.

I've avoided String.matches() / regex due to readability and performance. Also changedKeys.contains("eureka.client.region") is checked first since it's a O(1) operation. Kindly have a look.

[spencergibb]

Can you explain? Should go as a comment.

[fahimfarookme]

This check is there because if the the property eureka.client.use-dns-for-fetching-service-urls is set to true, service-urls will not be fetched from environment. I've documented it in the Java doc. Will add as a comment here as well.

[spencergibb]

Why throw an exception? Use an if.

[fahimfarookme]

I used an assert here only to specify the assumption that property keys cannot be null, hence not doing a null check here before calling key.startsWith(). Since assertions are disabled in production, hope this should be fine? Kindly let me know.

[spencergibb]

In boot 2.0 use only the - case names.

[fahimfarookme]

Looks like the the current master is still not migrated to boot 2.0? Anyways let me keep only the - case names.

[fahimfarookme]

@spencergibb I've addressed the review comments. Please have a look. Will rebase once you reviewed.

[fahimfarookme]

@spencergibb @ryanjbaxter Appreciate if you could review and let me know if anything needs to be addressed.

[ryanjbaxter]

LGTM

[ryanjbaxter]

Just a small nit here, this if could be combined in the for loop below. Small optimization more than anything else.

[fahimfarookme]

@ryanjbaxter I've rebased it. Kindly let me know if anything is pending from my-side to proceed with merging.

[ryanjbaxter]

@fahimfarookme thanks. The only outstanding thing from my side was my comment #2455 (comment). I think this might make sense to do

[fahimfarookme]

Thanks @ryanjbaxter for the review. I did a quick micro-benchmark with JMH since it looks like the submitted PR could perform better than the suggestion.

In the benchmark results below:

• BenchmarkRunner.submitted - is my PR
• BenchmarkRunner.suggested - is what you have suggested. i.e.

for (final String key : changedKeys) {
	if (key.startsWith("eureka.client.service-url.") || 
		key.startsWith("eureka.client.availability-zones.") || 
		changedKeys.contains("eureka.client.region")) {
		return true;
	}
}

• BenchmarkRunner.possibleSuggestion - I also tried another version along the lines of what you've suggested. i.e.

for (final String key : changedKeys) {
	if (key.startsWith("eureka.client.service-url.") || 
		key.startsWith("eureka.client.availability-zones.") || 
		"eureka.client.region".equals(key)) {
		return true;
	}
}

Here's the JMH results.

1. Throughput (higher the better)

Benchmark	Mode	Cnt	Score	Error	Units
BenchmarkRunner.possibleSuggestion	thrpt	20	5267629.307	± 571685.517	ops/s
BenchmarkRunner.submitted	thrpt	20	8492321.724	± 2838354.868	ops/s
BenchmarkRunner.suggested	thrpt	20	3019842.359	± 796130.173	ops/s

2. Average time (lower the better)

Benchmark	Mode	Cnt	Score	Error	Units
BenchmarkRunner.possibleSuggestion	avgt	20	≈ 10⁻⁷		s/op
BenchmarkRunner.submitted	avgt	20	≈ 10⁻⁷		s/op
BenchmarkRunner.suggested	avgt	20	≈ 10⁻⁶		s/op

Based on the results above the submitted PR is already the optimized version. Your thoughts please...