-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
improve handling of large single-part s3 uploads #49352
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
945eef2 to
c4b57af
Compare
df82691 to
ef0ce7c
Compare
|
Failing checks are relevant, still working on this |
this should reduce potential memory issues if the limit is set very high Signed-off-by: Robin Appelman <[email protected]>
…ream size is known Signed-off-by: Robin Appelman <[email protected]>
Signed-off-by: Robin Appelman <[email protected]>
Signed-off-by: Robin Appelman <[email protected]>
ef0ce7c to
6cf66f9
Compare
|
re-requesting reviews since there were some significant changes |
| $stats = fstat($stream); | ||
| if (is_array($stats) && isset($stats['size'])) { | ||
| $size = $stats['size']; | ||
| $this->logger->warning("stream size $size"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debugging leftover? otherwise warning seems a bit high as log level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
juliusknorr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comment, otherwise still 👍
|
Can we backport this to stable30? |
|
/backport to stable30 |
This removes the risk of the memory size ballooning in cases where
putSizeLimithas been made very high (because the setup doesn't have a working multi-part upload for example)