Replace http util reverese proxy with custom request forwarding #436
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
phillebaba
force-pushed
the
feature/replace-reverse-proxy
branch
from
d10ca7a
to
24b3600
Compare
|
@phillebaba maybe time for a benchmark? |
|
Yup I am working on it, but things need to be worked on a bit. Currently doing it manually. Also using pprof to explore the call stack. |
| @@ -87,6 +86,7 @@ func NewRegistry(ociClient oci.Client, router routing.Router, opts ...Option) *R | |||
| r := &Registry{ | |||
| ociClient: ociClient, | |||
| router: router, | |||
| httpClient: &http.Client{}, | |||
There was a problem hiding this comment.
Should we initialize it with some sane default?
httpClient := &http.Client{
Transport: &http.Transport{
MaxIdleConns: 20,
MaxIdleConnsPerHost: 20,
},
}
|
This has been quite helpful for me: https://github.com/XenitAB/go-oidc-middleware/blob/main/.github/workflows/benchmark.yaml |
phillebaba
force-pushed
the
feature/replace-reverse-proxy
branch
from
24b3600
to
2ea1404
Compare
phillebaba
force-pushed
the
feature/replace-reverse-proxy
branch
from
2ea1404
to
548b6b1
Compare
|
Some testing shows that this does not increase the performance. Either way it is a good change which gives us more control on top of reducing complexity. |
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.
The current implementation with reverse proxy was overly complex. Partially this is due to the retry logic implemented, but also the details of how things are implemented in the package. We really do not need all of the features of reverse proxy as we are not forwarding generic requests that could in theory contain any type of data. Instead the requests forwarded are specific to the OCI distribution spec.
This is the reason for replacing reverse proxy with out own http client that forwards requests.
An additional aspect is that the std lib reverse proxy does not use splice or send file when copying the response body. This solution should in theory improve performance.