Async feign flow #511

thanhj · 2021-03-13T11:36:25Z

No description provided.

codecov · 2021-03-13T23:18:47Z

Codecov Report

Merging #511 (d585597) into 2.2.x (e3439f3) will increase coverage by 0.53%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##              2.2.x     #511      +/-   ##
============================================
+ Coverage     77.69%   78.22%   +0.53%     
- Complexity      521      560      +39     
============================================
  Files            71       74       +3     
  Lines          2089     2301     +212     
  Branches        290      319      +29     
============================================
+ Hits           1623     1800     +177     
- Misses          327      348      +21     
- Partials        139      153      +14

Impacted Files	Coverage Δ	Complexity Δ
...mework/cloud/openfeign/FeignClientFactoryBean.java	`71.09% <71.55%> (+2.32%)`	`88.00 <27.00> (+26.00)`
...eign/async/AsyncHttpClient5FeignConfiguration.java	`88.33% <88.33%> (ø)`	`7.00 <7.00> (?)`
...d/openfeign/support/FeignHttpClientProperties.java	`93.81% <94.73%> (+0.06%)`	`20.00 <2.00> (+2.00)`
...ork/cloud/openfeign/FeignClientsConfiguration.java	`95.34% <100.00%> (+0.11%)`	`17.00 <1.00> (+1.00)`
...amework/cloud/openfeign/FeignClientsRegistrar.java	`78.92% <100.00%> (+0.20%)`	`54.00 <0.00> (ø)`
...d/openfeign/async/AsyncFeignAutoConfiguration.java	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
...rk/cloud/openfeign/async/DefaultAsyncTargeter.java	`100.00% <100.00%> (ø)`	`2.00 <2.00> (?)`

thanhj · 2021-03-22T14:38:30Z

Hi @OlgaMaciaszek, any chance that you have checked this PR?

OlgaMaciaszek · 2021-04-15T09:25:10Z

Hi, @thanhj - will work on it now.

OlgaMaciaszek

Hello, @thanhj thanks for submitting the PR. Have added some comments regarding things that will need to be changed in order to go forward with it, however, we should probably hold on with it still, as I have noticed that AsyncFeign is still marked as @Experimental and I am not sure we will want to add support till that changes - discussing it with the team.

OlgaMaciaszek · 2021-04-15T09:36:39Z

...openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java

@@ -166,6 +190,36 @@ protected void configureFeign(FeignContext context, Feign.Builder builder) {
 		}
 	}

+	protected void configureFeign(FeignContext context, AsyncBuilder builder) {


The class practically duplicates entire methods, with only small differences. Please refactor it so that this does not happen. You could extract code to smaller methods, use a wrapping interface that could have Builder or AsyncBuilder delegate, or use a method with more general arguments and use instanced checks and casting to process the two types, etc.

It is true that there are many duplications, I will try to make it better.

...nfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java

OlgaMaciaszek · 2021-04-15T09:49:04Z

.../main/java/org/springframework/cloud/openfeign/async/AsyncHttpClient5FeignConfiguration.java

+import org.springframework.context.annotation.Configuration;
+
+/**
+ * @author Nguyen Ky Thanh


Add description to javadoc.

OlgaMaciaszek · 2021-04-15T09:50:56Z

.../main/java/org/springframework/cloud/openfeign/async/AsyncHttpClient5FeignConfiguration.java

+		}
+	}
+
+	private TlsStrategy clientTlsStrategy(boolean isDisableSslValidation) {


Some of the code is similar to what is used by the synchronous version. Could you think of a way of reusing the code instead of duplicating?

OlgaMaciaszek · 2021-04-15T09:51:19Z

...ud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/async/AsyncTargeter.java

+import org.springframework.cloud.openfeign.FeignContext;
+
+/**
+ * @author Nguyen Ky Thanh


Please add description to javadoc.

OlgaMaciaszek · 2021-04-15T09:51:56Z

...feign-core/src/main/java/org/springframework/cloud/openfeign/async/DefaultAsyncTargeter.java

+import org.springframework.cloud.openfeign.FeignContext;
+
+/**
+ * @author Nguyen Ky Thanh


Please add description to javadoc.

OlgaMaciaszek · 2021-04-15T10:02:17Z

...ore/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpClientProperties.java

+	/**
+	 * Enumeration of pooled connection re-use policies.
+	 */
+	public enum PoolReusePolicy {


We cannot remove public types that have already been released in a non-major release. For example, if we released with org.springframework.cloud.openfeign.support.FeignHttpClientProperties.Hc5Properties.PoolReusePolicy, we can reuse it or add a new type, but we cannot just remove it (by placing it as an inner type of a different class).

thanhj · 2021-04-15T12:23:11Z

Hello, @thanhj thanks for submitting the PR. Have added some comments regarding things that will need to be changed in order to go forward with it, however, we should probably hold on with it still, as I have noticed that AsyncFeign is still marked as @Experimental and I am not sure we will want to add support till that changes - discussing it with the team.

Yes, it is true. But I still hope to receive positive feedback from your team discussion. I believe the @Experimental is more about the class design, and I also have tested the performance of this approach and got it applied to our production.

OlgaMaciaszek · 2021-04-15T14:22:51Z

@thanhj we have just discussed it in the team. We've decided that we would not be merging it to the main SC OpenFeign repo until it's @Experimental in Feign. However, we do have an incubator org: https://github.com/spring-cloud-incubator, and we could add a separate plug-in module (would have to be a separate maven dependency) with just the AsyncFeign support there. As an example you can see, https://github.com/spring-cloud-incubator/spring-cloud-sleuth-otel that is an incubator module for a released project (https://github.com/spring-cloud/spring-cloud-sleuth). Please let me know if you would like to work on sth like this.

thanhj

Yes, I would like to. Please let me know how to contribute there. @olegz

...nfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java

OlgaMaciaszek · 2021-04-20T15:33:57Z

@thanhj I have created an incubator repo: https://github.com/spring-cloud-incubator/spring-cloud-openfeign-async and already set it up. Please create a PR with your changes. Please add the changes in the spring-cloud-openfeign-async-core module and use the org.springframework.cloud.openfeign.async.core package.

thanhj · 2021-04-26T09:00:25Z

Thanks @OlgaMaciaszek, I am on it.

thanhj · 2021-05-02T15:08:12Z

I am not very familiar with setting up a Spring library from scratch like this, so it may take me a bit more time to raise first PR.
Also, any suggestion or direction from you would be great, thanks.

OlgaMaciaszek · 2021-05-07T09:29:18Z

@thanhj Will give it some thought and get back to you with some guidelines.

Thanh Nguyen added 2 commits March 12, 2021 23:49

Adding async feign with AsyncDefault client underlying.

6996e8a

New async flow [Done], first version.

efa7efd

spring-cloud-issues added the waiting-for-triage label Mar 13, 2021

Fix checkstyle

80640e6

Thanh Nguyen added 2 commits March 14, 2021 17:16

Add Apache HC5 Async option.

d268012

Add more coverage

b6a4525

thanhj changed the title ~~Async feign flow 2.2.x~~ Async feign flow Mar 14, 2021

More tests for async flow.

d585597

OlgaMaciaszek requested changes Apr 15, 2021

View reviewed changes

OlgaMaciaszek added for team discussion enhancement New feature or request and removed waiting-for-triage labels Apr 15, 2021

OlgaMaciaszek added icebox and removed for team discussion labels Apr 15, 2021

thanhj commented Apr 15, 2021

View reviewed changes

...nfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java Show resolved Hide resolved

thanhj mentioned this pull request Apr 17, 2021

When does AsyncFeign finish the @Experimental period? OpenFeign/feign#1384

Closed

OlgaMaciaszek added the waiting for feedback label Apr 20, 2021

OlgaMaciaszek self-assigned this Apr 20, 2021

OlgaMaciaszek mentioned this pull request May 7, 2021

Add AsyncFeign experimental support #534

Open

galaxy-sea mentioned this pull request Feb 2, 2023

Are there any plans to support http/2.0 #809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async feign flow #511

Async feign flow #511

thanhj commented Mar 13, 2021

codecov bot commented Mar 13, 2021 •

edited

thanhj commented Mar 22, 2021

OlgaMaciaszek commented Apr 15, 2021

OlgaMaciaszek left a comment

OlgaMaciaszek Apr 15, 2021

thanhj Apr 15, 2021

OlgaMaciaszek Apr 15, 2021

OlgaMaciaszek Apr 15, 2021

OlgaMaciaszek Apr 15, 2021

OlgaMaciaszek Apr 15, 2021

OlgaMaciaszek Apr 15, 2021

thanhj commented Apr 15, 2021

OlgaMaciaszek commented Apr 15, 2021

thanhj left a comment •

edited

OlgaMaciaszek commented Apr 20, 2021

thanhj commented Apr 26, 2021

thanhj commented May 2, 2021

OlgaMaciaszek commented May 7, 2021

Async feign flow #511

Are you sure you want to change the base?

Async feign flow #511

Conversation

thanhj commented Mar 13, 2021

codecov bot commented Mar 13, 2021 • edited

Codecov Report

thanhj commented Mar 22, 2021

OlgaMaciaszek commented Apr 15, 2021

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

OlgaMaciaszek Apr 15, 2021

Choose a reason for hiding this comment

thanhj Apr 15, 2021

Choose a reason for hiding this comment

OlgaMaciaszek Apr 15, 2021

Choose a reason for hiding this comment

OlgaMaciaszek Apr 15, 2021

Choose a reason for hiding this comment

OlgaMaciaszek Apr 15, 2021

Choose a reason for hiding this comment

OlgaMaciaszek Apr 15, 2021

Choose a reason for hiding this comment

OlgaMaciaszek Apr 15, 2021

Choose a reason for hiding this comment

thanhj commented Apr 15, 2021

OlgaMaciaszek commented Apr 15, 2021

thanhj left a comment • edited

Choose a reason for hiding this comment

OlgaMaciaszek commented Apr 20, 2021

thanhj commented Apr 26, 2021

thanhj commented May 2, 2021

OlgaMaciaszek commented May 7, 2021

codecov bot commented Mar 13, 2021 •

edited

thanhj left a comment •

edited