-
Notifications
You must be signed in to change notification settings - Fork 485
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
Limit number of the transformations threads #235
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #235 +/- ##
==========================================
+ Coverage 89.16% 89.34% +0.17%
==========================================
Files 6 6
Lines 674 685 +11
==========================================
+ Hits 601 612 +11
Misses 50 50
Partials 23 23
Continue to review full report at Codecov.
|
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Hey @striker2000, thanks for this patch! Sorry it's taken a couple of week to get back to you. I think this is certainly a good approach; my only reservation is about where exactly inside of Does that make sense? |
We're seeing fairly frequent OOM's (~10 a day) which are likely related to transformations. Is anyone using this patch successfully? |
@mdkent thsi PR has a problem with inflight request. I have updated with the current main branch and this PR. https://github.com/Jorgevillada/imageproxy
Some request never finish. |
When a search bot requests many images at the same time and these images are not in the cache, all transformation goroutines start in parallel causing the huge memory allocation, that leads to the termination of the process by OOM.
This fix is adding the limiter in the
TransformingTransport
that limits the number of running transformation threads to the number of logical CPUs. The metrichttp_requests_in_flight
is adding as well to monitor the length of the requests queue.Fixes #200.