-
Notifications
You must be signed in to change notification settings - Fork 310
Reuse transport of registry-scanner #1215
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1215 +/- ##
==========================================
+ Coverage 70.58% 70.79% +0.21%
==========================================
Files 45 45
Lines 5133 5178 +45
==========================================
+ Hits 3623 3666 +43
- Misses 1344 1346 +2
Partials 166 166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3dc05d0
to
89b08ed
Compare
89b08ed
to
79eef99
Compare
79eef99
to
188cbb7
Compare
@rainsun thanks for the contribution! Have you considered using How do you handle cache entry expiration and evication to avoid the cache growing indefinitely? What if the transport returned from the cache is already timed out and not usable? |
Thanks for the transport reuse/timeouts — big win under load. Do - JWT/token requests (e.g.
and also - perhaps retry could be introduced to the transport? |
d48b361
to
26f53a9
Compare
@chengfang Thanks for the insightful review and great suggestions! You raised some very valid points about the initial implementation. I've updated the code to address your feedback:
I also wanted to add a quick note on why I created a new, separate cache here instead of trying to reuse the one from pkg/cache/memcache.go. I figured it was better to keep them separate since they are caching two very different things (http.Transport vs image tags). They also have different requirements, especially Please let me know if this line of reasoning makes sense to you. Thanks again for your help |
From https://github.com/argoproj-labs/argocd-image-updater/blob/master/registry-scanner/pkg/registry/client.go#L113-L117 it is using the same Exposing certain options would require more thinking and I think would be beyond the scope of this pr.
http.Transport already handles retries, with certain restriction (from docs in http.Transport): |
Thanks @rainsun for updating the PR. It looks good overall and I added a comment about reduced timeout values. Can you please take a look? Also can you also add unit tests to cover the new functionalities? |
26f53a9
to
7a5f9bf
Compare
Thanks @chengfang for the review! I've reverted the timeout values to their defaults as you suggested. also added unit tests to cover the new transport caching functionality:
All tests are passing. Happy to hear any other thoughts you might have. |
…ry updates - Added transport caching to optimize HTTP transport reuse for registry endpoints. - Implemented ClearTransportCache function to clear the transport cache when registry configurations change, ensuring fresh transports are used. - Enhanced GetTransport method to utilize cached transports, improving performance and reducing overhead. Signed-off-by: rainsun <[email protected]>
7a5f9bf
to
847f803
Compare
feat(registry): implement transport caching and clear cache on registry updates
- Added transport caching to optimize HTTP transport reuse for registry endpoints.
- Implemented ClearTransportCache function to clear the transport cache when registry configurations change, ensuring fresh transports are used.
- Enhanced GetTransport method to utilize cached transports, improving performance and reducing overhead.
- fixes: #416