Support Spring RestClient as TransportClientFactory #4281
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`
| final Function<UriBuilder, URI> uriFunction = builder -> builder.pathSegment("apps", appName, id) | ||
| .queryParam("status", info.getStatus().toString()) | ||
| .queryParam("lastDirtyTimestamp", info.getLastDirtyTimestamp().toString()).queryParamIfPresent( | ||
| "overriddenstatus", Optional.ofNullable(overriddenStatus).map(InstanceStatus::name)) | ||
| .build(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Related #4163, so I guess it should be accepted as there's a linked bug report.
There was a problem hiding this comment.
Seems like a good idea.
|
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.
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.
Thanks @heowc. Have added some comments - please take a look. Will continue this review tomorrow.
| @@ -69,8 +74,6 @@ public EurekaClientConfigBean eurekaClientConfigBean() { | |||
|
|
|||
| @Bean | |||
| @ConditionalOnMissingBean(EurekaHttpClient.class) | |||
| @ConditionalOnProperty(prefix = "eureka.client", name = "webclient.enabled", matchIfMissing = true, | |||
There was a problem hiding this comment.
From my analysis, it seems that previously, resttemplate was set as default even if webclient was disabled. However, after that, I felt that the above setting was a bit ambiguous because there are two options even if webclient is disabled. What do you think? 🤔
There was a problem hiding this comment.
We want to preserve the default behaviour and only add opt-in RestClient support at this point. We may want to change it in a major release at this point. If webClient is not enabled, we want to use RestTemplate, and there should be a similar @Conditional added for restClient as well.
| @@ -68,8 +71,6 @@ public TlsProperties tlsProperties() { | |||
| @ConditionalOnClass(name = "org.springframework.web.client.RestTemplate") | |||
| @ConditionalOnMissingClass("org.glassfish.jersey.client.JerseyClient") | |||
| @ConditionalOnMissingBean(value = { AbstractDiscoveryClientOptionalArgs.class }, search = SearchStrategy.CURRENT) | |||
| @ConditionalOnProperty(prefix = "eureka.client", name = "webclient.enabled", matchIfMissing = true, | |||
| @@ -84,8 +85,6 @@ public RestTemplateDiscoveryClientOptionalArgs restTemplateDiscoveryClientOption | |||
| @ConditionalOnClass(name = "org.springframework.web.client.RestTemplate") | |||
| @ConditionalOnMissingClass("org.glassfish.jersey.client.JerseyClient") | |||
| @ConditionalOnMissingBean(value = { TransportClientFactories.class }, search = SearchStrategy.CURRENT) | |||
| @ConditionalOnProperty(prefix = "eureka.client", name = "webclient.enabled", matchIfMissing = true, | |||
| @@ -84,8 +85,6 @@ public RestTemplateDiscoveryClientOptionalArgs restTemplateDiscoveryClientOption | |||
| @ConditionalOnClass(name = "org.springframework.web.client.RestTemplate") | |||
| @ConditionalOnMissingClass("org.glassfish.jersey.client.JerseyClient") | |||
| @ConditionalOnMissingBean(value = { TransportClientFactories.class }, search = SearchStrategy.CURRENT) | |||
| @ConditionalOnProperty(prefix = "eureka.client", name = "webclient.enabled", matchIfMissing = true, | |||
There was a problem hiding this comment.
A similar conditional for restclient.enabled should be added.
| @@ -68,8 +71,6 @@ public TlsProperties tlsProperties() { | |||
| @ConditionalOnClass(name = "org.springframework.web.client.RestTemplate") | |||
| @ConditionalOnMissingClass("org.glassfish.jersey.client.JerseyClient") | |||
| @ConditionalOnMissingBean(value = { AbstractDiscoveryClientOptionalArgs.class }, search = SearchStrategy.CURRENT) | |||
| @ConditionalOnProperty(prefix = "eureka.client", name = "webclient.enabled", matchIfMissing = true, | |||
There was a problem hiding this comment.
A similar conditional for restclient.enabled should be added.
| search = SearchStrategy.CURRENT) | ||
| public RestClientDiscoveryClientOptionalArgs restClientDiscoveryClientOptionalArgs(TlsProperties tlsProperties, | ||
| ObjectProvider<RestClient.Builder> builder) throws GeneralSecurityException, IOException { | ||
| logger.info("Eureka HTTP Client uses RestClient."); |
There was a problem hiding this comment.
Wrap with a logger.isInfoEnabled() check. Could you also add it to the other calls of this method in the class?
| @@ -69,8 +74,6 @@ public EurekaClientConfigBean eurekaClientConfigBean() { | |||
|
|
|||
| @Bean | |||
| @ConditionalOnMissingBean(EurekaHttpClient.class) | |||
| @ConditionalOnProperty(prefix = "eureka.client", name = "webclient.enabled", matchIfMissing = true, | |||
There was a problem hiding this comment.
A similar conditional for restclient.enabled should be added.
| import static com.netflix.discovery.shared.transport.EurekaHttpResponse.anEurekaHttpResponse; | ||
|
|
||
| /** | ||
| * @author Wonchul Heo |
There was a problem hiding this comment.
Please add class description in the javadoc.
| final Function<UriBuilder, URI> uriFunction = builder -> builder.pathSegment("apps", appName, id) | ||
| .queryParam("status", info.getStatus().toString()) | ||
| .queryParam("lastDirtyTimestamp", info.getLastDirtyTimestamp().toString()).queryParamIfPresent( | ||
| "overriddenstatus", Optional.ofNullable(overriddenStatus).map(InstanceStatus::name)) | ||
| .build(); |
There was a problem hiding this comment.
Seems like a good idea.
|
Fixes gh-4257. |
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * @author Wonchul Heo | ||
| */ |
There was a problem hiding this comment.
Please add description in the class javadoc.
.../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
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
| /** | ||
| * @author Wonchul Heo | ||
| */ | ||
| class RestClientTransportClientFactoryTest { |
There was a problem hiding this comment.
Please rename to RestClientTransportClientFactoryTests.
| transportClientFactory = new RestClientTransportClientFactory(RestClient::builder); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
All the tests methods need some kind of assertions to verify that whatever was expected to happen has, in fact, happened.
| } | ||
|
|
||
| @Test | ||
| void testInvalidUserInfo() { |
There was a problem hiding this comment.
Let's remove the test prefix from all the test method names.
...va/org/springframework/cloud/netflix/eureka/http/RestTemplateTransportClientFactoryTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void testWithoutUserInfo() { | ||
| transportClientFatory.newClient(new DefaultEndpoint("http://localhost:8761")); | ||
| transportClientFactory.newClient(new DefaultEndpoint("http://localhost:8761")); |
There was a problem hiding this comment.
I know this code was already here, but as you're changing this area, can you please add appropriate assertions to all the test methods? and remove test prefix from the test method names?
Add related classes
RestClientDiscoveryClientOptionalArgsRestClientEurekaHttpClientRestClientTransportClientFactoriesRestClientTransportClientFactoryAdd property
eureka.client.restclient.enabled(defaultfalse)Change auto-configure
RestTemplate-related settings forwebclient.enabled=falsehave been removed.restclient.enabledmakes it meaninglessMove commonly used classes
NotFoundHttpResponseEurekaHttpClientUtilsEdit the document
Added similar tests referring to
RestTemplateandWebClientClose #4257