-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement new support packet API #24
Conversation
} | ||
defer fr.Close() | ||
|
||
b, err := io.ReadAll(fr) |
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.
Are we sure this won't cause OOM issues? Ideally we should stream the file contents downstream right?
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.
Yes, there is a small possibility but this was how the API being implemented, we need to change the API to accept something like a io.ReaderCloser
. However or in our case; ideally server can directly read from the filestore but still the API needs to be changed.
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.
Ok, wondering if we should file a ticket to track this? For example, k8s pods have a mem limit to them. There could be cases where this might be problematic. I'll approve for now but we should probably have a streaming API.
Summary
We can now safely use new support packet API.