Implement tests for EurekaReactiveDiscoveryClient and EurekaDiscoveryClientTests

[darth-raijin]

Changes:

• Add unit tests to EurekaReactiveDiscoveryClientTests.
• Add new EurekaDiscoveryClientTests focused on the default implementation for the probe() method.

See #4157 (comment)

[ryanjbaxter]

The default implementation already calls getServices() which calls this.eurekaClient.getApplications() so how is this doing anything different?

[darth-raijin]

Hey Ryan, thanks for reviewing :)

As per the Discovery client interface, where the probe() function is defined, the Javadocs mention there’s an intention for it to be a lightweight probe to ensure connectivity.

The major difference here is that calling this.eurekaClient.getApplications() directly avoids the overhead from getServices(), which iterates over all registered applications, allocates new lists, and lowercases each name. The goal was to keep probe() minimal and avoid unnecessary allocations when we don’t need the data being returned.

Do you prefer keeping it to getServices()?

[ryanjbaxter]

Agree getServices() does do some additional work, but most of the overhead is incurred by the network request of the getApplications() call itself so I dont see this saving a ton of overhead unless you had a very large number of instances.

[darth-raijin]

That makes sense, this might be a premature/uneeded optimization. I am fine with reverting the override, and keeping the default implementation.

What do you think would be a fitting way to move forward?

[ryanjbaxter]

I don't think we need to do anything because it should already be using the default implementation

[darth-raijin]

The override is now reverted as discussed

[ryanjbaxter]

I guess I dont understand why we need to make any changes in spring-cloud-netflix if the default implementations in spring-cloud-commons is acceptable

[darth-raijin]

Apologies, I seem to have misunderstood the intention :)
Do you think the tests would still add any value, or should I go ahead and close the PR? Thanks again for taking the time to review it!

[ryanjbaxter]

The tests are fine but the reactive implementation is still overriding the default implementation