Phil/http client errors #369
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rom cloud storage" This reverts commit 81d0aa5. The intent of that commit was to work around an issue where a bad connection could be reused for subsequent requests. But I now think that this brings too great a risk for performance degradation due to the much lower effective connection limits. And I also suspect it may not even solve the problem, since http1.1 connections are still re-used in many cases. So the plan is to try something different in the next commit.
psFried
force-pushed
the
phil/http-client-errors
branch
2 times, most recently
from
b967eb0 to
9fc7b41
Compare
This addresses an issue we've encountered in production that results in a large number of request failures. This happens when one of the TCP connections is to a server that starts sending RSTStream frames with an `INTERNAL_ERROR` status (http2 stream status, not http request status). In this case, the Go http client will continue to re-use the connection for other requests, which also fail in the same way. What we'd really like is to have some sort of logic that says, "if connection x has returned such an internal error, then stop using that connection for future requests and shut it down". Unfortunately, such logic is impossible to express with the current `Transport` and `ClientConnPool` APIs. So the next best thing is a workaround where we re-create the entire http client, to force it to re-establish all connections. That's what this does. Plus a little extra for dealing with the dynamic enablement/disablement of the file protocol transport.
psFried
force-pushed
the
phil/http-client-errors
branch
from
9fc7b41 to
e9352e8
Compare
psFried
added a commit
to estuary/flow
that referenced
this pull request
Mar 15, 2024
psFried
added a commit
to estuary/flow
that referenced
this pull request
Mar 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses an issue we've encountered in production that results in a large
number of request failures. This happens when one of the TCP connections is to
a server that starts sending RSTStream frames with an
INTERNAL_ERRORstatus(http2 stream status, not http request status). In this case, the Go http
client will continue to re-use the connection for other requests, which also
fail in the same way. What we'd really like is to have some sort of logic that
says, "if connection x has returned such an internal error, then stop using
that connection for future requests and shut it down". Unfortunately, such
logic is impossible to express with the current
TransportandClientConnPoolAPIs. So the next best thing is a workaround where were-create the entire http client, to force it to re-establish all connections.
That's what this does. Plus a little extra for dealing with the dynamic
enablement/disablement of the file protocol transport.
The previous attempt to work around this issue was to force the client to use http1.1 only. It's unclear whether that would have worked, or if it would have experienced the same issue just with http 500 responses. In any case, this reverts that commit in favor of re-creating the client only when failures are detected.
I tested this by running
gazctl journals readand verifying via the debug logs that fragments were being fetched directly.This change is