Skip to content
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(api): remove protocol file size limit and ack immediately #4006

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

btmorr
Copy link
Contributor

@btmorr btmorr commented Sep 5, 2019

Fixes #3998

An arbitrary (default) file size limit in the API made it impossible to upload protocol files larger than 4Mb. After removing that limit, we discovered that:

  1. The API simulates the protocol before sending an 'ack' back to the client, causing the client request to time out
  2. The API had log statments that would crash the API server when receiving large files

This PR removes the log statements from the API (and a log statement from the App with the same problem), and removes the file size limit. It also moves the 'ack' response to before simulation, as expected by the App.

@btmorr btmorr requested a review from mcous September 5, 2019 20:07
@btmorr btmorr added api Affects the `api` project fix PR fixes a bug labels Sep 5, 2019
@btmorr
Copy link
Contributor Author

btmorr commented Sep 5, 2019

As a follow-up, we should do a performance test on the impact of other logging statements in the API during simulation.

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

Merging #4006 into edge will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #4006      +/-   ##
==========================================
- Coverage   57.49%   57.48%   -0.02%     
==========================================
  Files         828      828              
  Lines       23478    23470       -8     
==========================================
- Hits        13499    13491       -8     
  Misses       9979     9979
Impacted Files Coverage Δ
app/src/rpc/client.js 89.79% <ø> (ø) ⬆️
app/src/robot/api-client/index.js 11.11% <0%> (ø) ⬆️
opentrons/server/rpc.py 89.06% <0%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 921e2cb...b2fd0bc. Read the comment docs.

@mcous mcous added app Affects the `app` project rpc This involves Opentrons' deprecated RPC system. labels Sep 6, 2019
@mcous mcous requested a review from a team September 6, 2019 16:23
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Code looks good and behavior LGTM. Awaiting @Opentrons/py approval before merging

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

lgtm

@btmorr btmorr merged commit 2a82724 into edge Sep 9, 2019
@btmorr btmorr deleted the api_remove-rpc-file-limit branch September 9, 2019 14:05
@mcous mcous mentioned this pull request Sep 9, 2019
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project app Affects the `app` project fix PR fixes a bug rpc This involves Opentrons' deprecated RPC system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading .json protocol results in robot server crashing
3 participants