Skip to content

Conversation

@galaxy-sea
Copy link
Contributor

@szpak
Copy link

szpak commented Dec 30, 2022

I had bumped into that problem recently and I've just wanted to report that future request :-).

It is especially useful, as "total" timeout to perform the request-response pair doesn't seem to be supported by HC5 - https://issues.apache.org/jira/browse/HTTPCLIENT-1074

@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request in progress and removed waiting-for-triage labels Jan 16, 2023
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @galaxy-sea, thanks for submitting this PR. I've added a comment - please address it. Also, please submit these changes against 3.1.x instead of main, so we'll get them included in the 2021.x release train as well.

/**
* Default value for connection request timeout.
*/
public static final int DEFAULT_CONNECTION_REQUEST_TIMEOUT = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to maintain compatibility, let's stick with the HC5 defualt connection request timeout (3 minutes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Merging #810 (bd232c7) into main (80c8055) will increase coverage by 0.14%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #810      +/-   ##
============================================
+ Coverage     74.44%   74.59%   +0.14%     
  Complexity      570      570              
============================================
  Files            67       67              
  Lines          2211     2224      +13     
  Branches        300      300              
============================================
+ Hits           1646     1659      +13     
  Misses          396      396              
  Partials        169      169              
Impacted Files Coverage Δ
...gn/clientconfig/HttpClient5FeignConfiguration.java 69.56% <100.00%> (+2.89%) ⬆️
...d/openfeign/support/FeignHttpClientProperties.java 93.90% <100.00%> (+0.75%) ⬆️

@galaxy-sea
Copy link
Contributor Author

Hello @galaxy-sea, thanks for submitting this PR. I've added a comment - please address it. Also, please submit these changes against 3.1.x instead of main, so we'll get them included in the 2021.x release train as well.

hello @OlgaMaciaszek, I resubmitted a PR for the 3.1. x branch . pr#817

@OlgaMaciaszek
Copy link
Collaborator

Thanks @galaxy-sea . Closing in favour of #817.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants