-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Support Spring RestClient as TransportClientFactory #4281
base: main
Are you sure you want to change the base?
Conversation
Add related classes - `RestClientDiscoveryClientOptionalArgs` - `RestClientEurekaHttpClient` - `RestClientTransportClientFactories` - `RestClientTransportClientFactory` Add property - `eureka.client.restclient.enabled` (default `false`) Change auto-configure - `RestTemplate`-related settings for `webclient.enabled=false` have been removed. `restclient.enabled` makes it meaningless Move commonly used classes - `NotFoundHttpResponse` - `EurekaHttpClientUtils` Edit the document Added similar tests referring to `RestTemplate` and `WebClient`
.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java
Outdated
Show resolved
Hide resolved
Could you please inform me if there are any plans to phase out |
FYI) target issue: #4257 |
gentle ping 😄 |
Thanks @heowc, will take a look. We do not have a plan for phasing out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @heowc. So far, have only looked at the docs - please implement the suggestion. Will continue the review of this PR tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @heowc. Have added some comments - please take a look. Will continue this review tomorrow.
...rg/springframework/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfiguration.java
Outdated
Show resolved
Hide resolved
...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java
Show resolved
Hide resolved
...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java
Outdated
Show resolved
Hide resolved
...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java
Outdated
Show resolved
Hide resolved
...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java
Show resolved
Hide resolved
...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java
Outdated
Show resolved
Hide resolved
...rg/springframework/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfiguration.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java
Show resolved
Hide resolved
.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java
Outdated
Show resolved
Hide resolved
Fixes gh-4257. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on it @heowc. This concludes the first review pass on this PR. Please address the comments I've added and let me know when it's ready for another pass.
Also, please make sure to update the date in license file comments in all the classes you've modified to end with -2022
, add your full name and surname with the @author
tag to the javadocs to all the classes you've modified, and add @since 4.1.3
to the javadocs of all the production classes you've added.
...ava/org/springframework/cloud/netflix/eureka/http/RestClientDiscoveryClientOptionalArgs.java
Show resolved
Hide resolved
return getApplicationsInternal("/apps/", regions); | ||
} | ||
|
||
private EurekaHttpResponse<Applications> getApplicationsInternal(String urlPath, String[] regions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the private methods below the public ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move all the private methods below all the public ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is the same for all other implementations. Not only that, but other methods (ex. getInstanceInternal) are in a similar situation.
Isn't this the original intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally try to keep the public API on top; sometimes things get missed while adding code and then spotted while revisiting a fragment with new changes; it's never a bad idea to add an improvement as we go.
.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java
Show resolved
Hide resolved
...ain/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java
Show resolved
Hide resolved
...lient/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaHttpClientUtils.java
Show resolved
Hide resolved
...java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryTest.java
Outdated
Show resolved
Hide resolved
...java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryTest.java
Outdated
Show resolved
Hide resolved
...java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryTest.java
Outdated
Show resolved
Hide resolved
...va/org/springframework/cloud/netflix/eureka/http/RestTemplateTransportClientFactoryTest.java
Outdated
Show resolved
Hide resolved
...va/org/springframework/cloud/netflix/eureka/http/RestTemplateTransportClientFactoryTest.java
Outdated
Show resolved
Hide resolved
# Conflicts: # docs/modules/ROOT/partials/_configprops.adoc # spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java # spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/RestTemplateTransportClientFactory.java # spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/config/EurekaHttpClientsOptionalArgsConfigurationTests.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return getApplicationsInternal("/apps/", regions); | ||
} | ||
|
||
private EurekaHttpResponse<Applications> getApplicationsInternal(String urlPath, String[] regions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move all the private methods below all the public ones.
final ResponseEntity<String> response = restClient.get().uri("http://localhost:" + port).retrieve() | ||
.toEntity(String.class); | ||
|
||
assertThat(response).isNotNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that we're testing a call done by RestClient
, which is a component provided by Spring Framework and tested very well itself. What we should do is instead use RestClientEurekaHttpClient
to do the call, and then verify its result.
...ork/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfigurationRestClientTests.java
Show resolved
Hide resolved
@@ -204,4 +210,32 @@ static class OnJerseyClientDisabled { | |||
|
|||
} | |||
|
|||
@ConditionalOnMissingClass("org.glassfish.jersey.client.JerseyClient") | |||
@ConditionalOnClass(name = "org.springframework.web.client.RestClient") | |||
@ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some more consideration, I actually think we could switch to using RestClient
by default in case of the non-blocking stack, so let's add matchIfMissing=true
and @ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "false")
to the RestTemplate
-based configuration. Sorry for the confusion.
@@ -109,6 +114,22 @@ public WebClientEurekaHttpClient configDiscoveryWebClientEurekaHttpClient(Eureka | |||
|
|||
} | |||
|
|||
@Configuration(proxyBeanMethods = false) | |||
@ConditionalOnClass(name = "org.springframework.web.client.RestClient") | |||
@ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some more consideration, I actually think we could switch to using RestClient
by default in case of the non-blocking stack, so let's add matchIfMissing=true
and @ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "false")
to the RestTemplate
-based configuration. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added related conditions while changing JerseyClientNotPresentOrNotEnabledCondition
to RestTemplateEnabledCondition
. Is this what you intended? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the logic after your changes has been modified - please take a look at the review comments.
...etflix/eureka/config/EurekaConfigServerBootstrapConfigurationRestClientIntegrationTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @heowc. Have added additional comments. Please take a look.
@@ -73,10 +72,8 @@ public TlsProperties tlsProperties() { | |||
|
|||
@Bean | |||
@ConditionalOnClass(name = "org.springframework.web.client.RestTemplate") | |||
@Conditional(JerseyClientNotPresentOrNotEnabledCondition.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still want to keep the JerseyClientNotPresentOrNotEnabledCondition
here.
@@ -117,7 +114,7 @@ private static void setupTLS(AbstractDiscoveryClientOptionalArgs<?> args, TlsPro | |||
} | |||
|
|||
@Configuration(proxyBeanMethods = false) | |||
@Conditional(JerseyClientPresentAndEnabledCondition.class) | |||
@Conditional(RestTemplateEnabledCondition.class) | |||
@ConditionalOnBean(value = AbstractDiscoveryClientOptionalArgs.class, search = SearchStrategy.CURRENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still want to keep the JerseyClientNotPresentOrNotEnabledCondition
here, whether or not RestTemplate
is enabled.
super(ConfigurationPhase.REGISTER_BEAN); | ||
} | ||
|
||
@ConditionalOnClass(name = "org.glassfish.jersey.client.JerseyClient") | ||
static class OnJerseyClientPresent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Jersey conditions are not related to the WebClient
/ RestClient
/ RestTemplate
selection. They should be separate.
class EurekaConfigServerBootstrapConfigurationClientTests { | ||
|
||
@Test | ||
void properBeansCreatedWhenRestClientEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's also add one that tests the RestTemplate
-backed client bean created and both RestClient
and WebClient
- backed beans missing.
Add related classes
RestClientDiscoveryClientOptionalArgs
RestClientEurekaHttpClient
RestClientTransportClientFactories
RestClientTransportClientFactory
Add property
eureka.client.restclient.enabled
(defaultfalse
)Move commonly used classes
NotFoundHttpResponse
EurekaHttpClientUtils
Edit the document
Added similar tests referring to
RestTemplate
andWebClient
Close #4257