-
Notifications
You must be signed in to change notification settings - Fork 352
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
Fix OOM lakectl FS upload bug #8349
base: master
Are you sure you want to change the base?
Conversation
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.
Could you explain how this PR solves the linked issue?
@itaiad200 Yes, follow me- From the uploadObject/UploadPartObject we have this code:
the type of the body we pass to NewRequestWithContext is local.fileWrapper which implement seek but doesn't implement close
So we get a request from body that implements close (NopCloser) but doesn't implement seek Now when getting to this line- putResp, err := u.uploader.HTTPClient.Do(req)
Now, because our body doesn't implement seek but does implement read, we will call io.ReadAll and read the entire file into the memory and crush. |
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.
NICE Research!
Sometimes small solutions solve big problems :)
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.
Thanks for the clear explanation. How was this tested?
@itaiad200 I built a docker image that runs lakectl fs upload and tries to upload a 1GB file using the --memory=512m flag. |
Closes #8088