Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Hotfix] Fix file upload limit for gRPC and flask #35

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

keigohtr
Copy link
Member

What is this PR for?

Modify the file upload limit for gRPC and flask.
Since default gRPC settings limits 4MB data for transference in an one connection, we need to fix our file transference setting.

This PR includes

  • Specify file upload limit for flask
  • Specify file transference limit for gRPC

What type of PR is it?

Bugfix

What is the issue?

N/A

How should this be tested?

Upload ML model more than 4MB.

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2018

CLA assistant check
All committers have signed the CLA.

@@ -19,6 +19,6 @@ def serialize_to_object(proto, descriptor = None):
return result

@staticmethod
def stream_file(f:FileStorage, size:int=100*1024*1024) -> bytes:
def stream_file(f:FileStorage, size:int=4190000) -> bytes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this value looks arbitrary...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upper transference limit is 4MB, so we cannot occupy 4MB for file transference. The rest band is for head info and so on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so we have 4304 bytes left for header info which seems enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
I didn't calculate an edge case. I believe that about 4KB is enough for the other info.

Copy link
Member

@yoquankara yoquankara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -19,6 +19,6 @@ def serialize_to_object(proto, descriptor = None):
return result

@staticmethod
def stream_file(f:FileStorage, size:int=100*1024*1024) -> bytes:
def stream_file(f:FileStorage, size:int=4190000) -> bytes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so we have 4304 bytes left for header info which seems enough.

@keigohtr
Copy link
Member Author

keigohtr commented Oct 1, 2018

Thank you!

@keigohtr keigohtr merged commit 7909be3 into master Oct 1, 2018
@keigohtr keigohtr deleted the hotfix/fix-fileupload-limit branch October 1, 2018 08:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants