Skip to content
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

Reuse connections for 5xx errors #5607

Merged
merged 4 commits into from
Jan 22, 2025

Conversation

zoewangg
Copy link
Contributor

Motivation and Context

Reuse connections for 5xx errors for all supported HTTP clients. This change reverts #1768 and #2960

@jtjeferreira
Copy link

Hi @zoewangg,

I am working on an AWS SDK Http client based on pekko here, and the only failing test from SdkAsyncHttpClientH1TestSuite is connectionReceiveServerErrorStatusShouldNotReuseConnection which I am ignoring for now.

Luckily I stumbled upon this PR, and I am just wondering why this functionality is being reverted?

@zoewangg
Copy link
Contributor Author

Hi @jtjeferreira, we are still evaluating the change. The main reason is that closing connections for 5xx errors could cause more TCP connection establishment in the unlikely event of outage, which could add request latency and add more load on the server side. In general, we try to keep custom logic in the SDK minimal and let the server side decide if a connection should be kept open or not.

@zoewangg zoewangg force-pushed the zoewang/master/reuseConnectionFor5xxErrors branch from b9165da to b8a4171 Compare January 20, 2025 21:36
@zoewangg zoewangg marked this pull request as ready for review January 20, 2025 21:36
@zoewangg zoewangg requested a review from a team as a code owner January 20, 2025 21:36
@zoewangg zoewangg force-pushed the zoewang/master/reuseConnectionFor5xxErrors branch from b8a4171 to 0cd9322 Compare January 20, 2025 22:54
@zoewangg zoewangg enabled auto-merge January 21, 2025 20:59
@zoewangg zoewangg added this pull request to the merge queue Jan 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 21, 2025
@zoewangg zoewangg added this pull request to the merge queue Jan 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@zoewangg zoewangg added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@zoewangg zoewangg added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@zoewangg zoewangg enabled auto-merge January 22, 2025 03:48
Copy link

@zoewangg zoewangg added this pull request to the merge queue Jan 22, 2025
Merged via the queue into master with commit 0bef9fd Jan 22, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants