Support Spring RestClient as TransportClientFactory #4281

heowc · 2024-05-05T07:11:59Z

Change RestClient to default eureka client. This changes the default behavior.

Changed from RestTemplate to RestClient
Related tests have also been changed

Add related classes

RestClientDiscoveryClientOptionalArgs
RestClientEurekaHttpClient
RestClientTransportClientFactories
RestClientTransportClientFactory

Add property

eureka.client.restclient.enabled (default false)

Move commonly used classes

NotFoundHttpResponse
EurekaHttpClientUtils

Edit the document

Added similar tests referring to RestTemplate and WebClient

Close #4257

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

heowc · 2024-05-05T08:53:20Z

Could you please inform me if there are any plans to phase out RestTemplate related implementations in the future? 🤔

injae-kim · 2024-05-05T10:52:55Z

FYI) target issue: #4257

heowc · 2024-06-20T04:40:34Z

gentle ping 😄

OlgaMaciaszek · 2024-06-26T14:20:31Z

Thanks @heowc, will take a look. We do not have a plan for phasing out RestTemplate at this point.

OlgaMaciaszek

Thanks, @heowc. So far, have only looked at the docs - please implement the suggestion. Will continue the review of this PR tomorrow.

docs/modules/ROOT/pages/spring-cloud-netflix.adoc

OlgaMaciaszek

Thanks @heowc. Have added some comments - please take a look. Will continue this review tomorrow.

...rg/springframework/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfiguration.java

...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java

...rg/springframework/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfiguration.java

.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java

OlgaMaciaszek · 2024-07-04T09:28:55Z

Fixes gh-4257.

OlgaMaciaszek

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

.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java

...ain/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java

...lient/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaHttpClientUtils.java

...java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryTest.java

...va/org/springframework/cloud/netflix/eureka/http/RestTemplateTransportClientFactoryTest.java

OlgaMaciaszek · 2024-07-08T14:36:35Z

Hi @heowc, I have pushed a fix in the same area: b23c587. Please merge the code and resolve any conflict before proceeding with working on this PR.

# 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

OlgaMaciaszek

Thanks, @heowc. Have added some comments. Please take a look. Also, please merge the changes from origin/main, since I've modified some tests that may cause a conflict: 44e22ad.

.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java

OlgaMaciaszek · 2024-10-09T15:49:19Z

...etflix/eureka/config/EurekaConfigServerBootstrapConfigurationRestClientIntegrationTests.java

+		final ResponseEntity<String> response = restClient.get().uri("http://localhost:" + port).retrieve()
+				.toEntity(String.class);
+
+		assertThat(response).isNotNull();


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

...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java

OlgaMaciaszek · 2024-10-09T16:00:44Z

...rg/springframework/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfiguration.java

@@ -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")


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.

I added related conditions while changing JerseyClientNotPresentOrNotEnabledCondition to RestTemplateEnabledCondition. Is this what you intended? 🤔

No, the logic after your changes has been modified - please take a look at the review comments.

I think I understood it a little differently, but I think you should change JerseyClientNotPresentOrNotEnabledCondition to RestTemplateEnabledCondition for the same issue. Don't you think so??

If there is something I misunderstood, please let me know.

Modify the eureka.client.restclient.enabled annotation I added as follows.

@ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "true")

->

@ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "true", matchIfMissing = true)

Then, set the following annotation to the configuration beans that are set based on RestTemplate.

@ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", havingValue = "false")

However, if I set it like this, some tests will fail. The reason is that matchIfMissing is changed so that it should default to RestTemplate, but it changes to RestClient, or even RestTemplate is not set, so the bean is not configured.

...etflix/eureka/config/EurekaConfigServerBootstrapConfigurationRestClientIntegrationTests.java

OlgaMaciaszek

Thanks @heowc. Have added additional comments. Please take a look.

...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java

...amework/cloud/netflix/eureka/config/EurekaConfigServerBootstrapConfigurationClientTests.java

OlgaMaciaszek

Hi @heowc, thanks for adding changes and comments. I have tried to further clarify the configuration setup by adding some comments to the code changes along with reasoning behind them. If possible, please add the changes discussed till the end of this week. If you don't have the time, no worries, however, since I want to make sure we get this feature into the upcoming RC, if this is not ready, then on Mon, I will cherry-pick your changes and add any necessary changes on top of them. Thank you for all your work on this PR (and others).

...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java

OlgaMaciaszek · 2024-10-31T11:11:04Z

...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java

+		}
+
+		@ConditionalOnProperty(prefix = "eureka.client", name = "restclient.enabled", matchIfMissing = true,
+				havingValue = "false")


This should not be matchIfMissing=true. Since we now want the RestClient based implementation to be default, we only want to do RestTemplate if RestClient is explicitly disabled.

I was really hoping that the default would change while working on this issue (it's too complicated).
Is this direction related to the issue below?
#4347

Yes, we would like to change the default here to RestClient. That is why we don't want to use RestTemplate unless RestClient is explicitly disabled.

...rg/springframework/cloud/netflix/eureka/config/DiscoveryClientOptionalArgsConfiguration.java

heowc · 2024-10-31T14:20:42Z

Hi @heowc, thanks for adding changes and comments. I have tried to further clarify the configuration setup by adding some comments to the code changes along with reasoning behind them. If possible, please add the changes discussed till the end of this week. If you don't have the time, no worries, however, since I want to make sure we get this feature into the upcoming RC, if this is not ready, then on Mon, I will cherry-pick your changes and add any necessary changes on top of them. Thank you for all your work on this PR (and others).

Thank you for reviewing this issue for a rather long time. I was fixed everything in response to your comments, but if you have any other suggestions, please let us know.

Of course, there were some test modifications with the changed direction. Please take note.

...ain/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java

heowc · 2024-11-03T04:16:52Z

Hello @OlgaMaciaszek,
As you mentioned, I've modified the setup, testing, and documentation to make RestClient the default client.

See #4281 (comment)

OlgaMaciaszek

LGTM, @heowc. Thanks for relentlessly working on this.

injae-kim · 2024-11-05T11:57:03Z

Nice work @heowc !!! also thanks for the kind review @OlgaMaciaszek 👍👍👍

spring-cloud-issues added the waiting-for-triage label May 5, 2024

heowc commented May 5, 2024

View reviewed changes

.../src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientEurekaHttpClient.java Outdated Show resolved Hide resolved

Revert RestTemplate-related DiscoveryClientOptionalArgsConfiguration

783d4c1

Cleanup

142f1af

injae-kim mentioned this pull request May 5, 2024

Support Spring RestClient as TransportClientFactory and make it the default implementation #4257

Closed

OlgaMaciaszek reviewed Jun 26, 2024

View reviewed changes

docs/modules/ROOT/pages/spring-cloud-netflix.adoc Outdated Show resolved Hide resolved

Fix docs

a9c6260

heowc requested a review from OlgaMaciaszek June 29, 2024 05:55

OlgaMaciaszek requested changes Jul 3, 2024

View reviewed changes

OlgaMaciaszek requested changes Jul 4, 2024

View reviewed changes

OlgaMaciaszek self-assigned this Jul 4, 2024

OlgaMaciaszek added enhancement and removed waiting-for-triage labels Jul 4, 2024

heowc added 5 commits October 3, 2024 23:03

Fix

760dae3

Fix

1084cbc

Fix

a6c2690

apply review

a9f8a23

heowc requested a review from OlgaMaciaszek October 6, 2024 08:08

OlgaMaciaszek mentioned this pull request Oct 9, 2024

Remove RestTemplate-based setup in favouf of RestClient-based one #4347

Open

OlgaMaciaszek requested changes Oct 9, 2024

View reviewed changes

heowc added 2 commits October 12, 2024 12:39

Merge branch 'spring-cloud:main' into feature/4257

6fd4f58

add test

0ea8039

heowc added 7 commits October 12, 2024 14:01

Remove unused import

ac24e37

Checkstyle

4c7ce21

Add RestTemplateEnabledCondition

871451c

Fix RestTemplateEnabledCondition

e239d9b

Checkstyle

8abfc6d

Fix test

8581ab0

Fix test

ffcff5d

OlgaMaciaszek requested changes Oct 14, 2024

View reviewed changes

heowc added 3 commits October 26, 2024 14:23

Fix order

de8cdb9

Fix

55de1f5

Fix

13e6718

OlgaMaciaszek requested changes Oct 31, 2024

View reviewed changes

Fix

e107f68

heowc requested a review from OlgaMaciaszek October 31, 2024 14:20

heowc commented Nov 1, 2024

View reviewed changes

...ain/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java Show resolved Hide resolved

Change default client

92ede0d

OlgaMaciaszek approved these changes Nov 4, 2024

View reviewed changes

OlgaMaciaszek merged commit c2c5352 into spring-cloud:main Nov 4, 2024
1 check passed

heowc deleted the feature/4257 branch November 5, 2024 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Spring RestClient as TransportClientFactory #4281

Support Spring RestClient as TransportClientFactory #4281

heowc commented May 5, 2024 •

edited

Loading

heowc commented May 5, 2024

injae-kim commented May 5, 2024

heowc commented Jun 20, 2024

OlgaMaciaszek commented Jun 26, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek left a comment

OlgaMaciaszek commented Jul 4, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek commented Jul 8, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek Oct 9, 2024

OlgaMaciaszek Oct 9, 2024

heowc Oct 12, 2024

OlgaMaciaszek Oct 14, 2024

heowc Oct 27, 2024

heowc Oct 27, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek left a comment

OlgaMaciaszek Oct 31, 2024

heowc Oct 31, 2024

OlgaMaciaszek Oct 31, 2024

heowc commented Oct 31, 2024

heowc commented Nov 3, 2024

OlgaMaciaszek left a comment

injae-kim commented Nov 5, 2024

Support Spring RestClient as TransportClientFactory #4281

Support Spring RestClient as TransportClientFactory #4281

Conversation

heowc commented May 5, 2024 • edited Loading

heowc commented May 5, 2024

injae-kim commented May 5, 2024

heowc commented Jun 20, 2024

OlgaMaciaszek commented Jun 26, 2024

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

OlgaMaciaszek commented Jul 4, 2024

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

OlgaMaciaszek commented Jul 8, 2024

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heowc commented Oct 31, 2024

heowc commented Nov 3, 2024

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

injae-kim commented Nov 5, 2024

heowc commented May 5, 2024 •

edited

Loading