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

Allow large form posts via multipart encoded forms to command API #5000

Merged
merged 2 commits into from Jan 22, 2019

Conversation

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 13, 2018

This temporarily puts this under a feature flag, to make debugging a
little easier, but initial tests show things working as expected. This
allows for a large form post to be chunked up and POST, where we were
noticing a number of different issues with body size before.

Additionally, we can remove the chunked encoding PR as this was not the ultimate fix that worked.

Hotfixed on readthedocsinc, not on community version.

This temporarily puts this under a feature flag, to make debugging a
little easier, but initial tests show things working as expected. This
allows for a large form post to be chunked up and POST, where we were
noticing a number of different issues with body size before.
@agjohnson agjohnson requested a review from Dec 13, 2018
@agjohnson agjohnson added this to the 2.9 milestone Dec 13, 2018
@codecov
Copy link

@codecov codecov bot commented Dec 13, 2018

Codecov Report

No coverage uploaded for pull request base (master@59d9e3e). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #5000   +/-   ##
=========================================
  Coverage          ?   76.72%           
=========================================
  Files             ?      158           
  Lines             ?     9962           
  Branches          ?     1246           
=========================================
  Hits              ?     7643           
  Misses            ?     1984           
  Partials          ?      335
Impacted Files Coverage Δ
readthedocs/restapi/views/model_views.py 95.18% <100%> (ø)
readthedocs/projects/models.py 85.58% <100%> (ø)
readthedocs/doc_builder/environments.py 85.27% <100%> (ø)

@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Dec 14, 2018

Also, this might be able to be implemented as a slumber serializer so we're not forking the code more here.

Docs at: https://slumber.readthedocs.io/en/v0.6.0/options.html#serializer

Perhaps we can clean up this hotfix PR with a follow up PR and a better serializer.

@humitos
Copy link
Member

@humitos humitos commented Dec 17, 2018

Also, this might be able to be implemented as a slumber serializer so we're not forking the code more here.

I like this idea. I'd wait some weeks to see if this solution solves most of these problems and in that case, I can write the serializer. I think it's the way to go.

Copy link
Member

@humitos humitos left a comment

Looks good!

I've triggered my project at corporate with huge build output and it built properly.

One problem that I noticed is that none of the sphinx-build commands were logged in the build output: https://readthedocs.com/projects/read-the-docs-test-builds/builds/168252/

I'm not sure if it's related to this, but it wasn't happening before this PR.

@humitos
Copy link
Member

@humitos humitos commented Dec 17, 2018

On the other hand, for a normal build without huge output, it did log those commands: https://readthedocs.com/projects/read-the-docs-test-builds/builds/168251/

@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Dec 19, 2018

Strange, i missed this in my testing, i don't see a build that has those steps. I might have solved the request issue but opened a new bug with this. I do think this is at least part of the solution and agree it probably makes sense to let this simmer and figure out if writing a serializer would further help 👍

I'd say we should merge this if it looks okay, as it is feature flagged anyways, and track down why the large command posts are not showing up in a separate issue

@humitos
Copy link
Member

@humitos humitos commented Dec 20, 2018

I might have solved the request issue but opened a new bug with this

What is strange to me is that all of the requests are done with the MultipartEncoder but only large ones are not logged :/

I'd say we should merge this if it looks okay, as it is feature flagged anyways, and track down why the large command posts are not showing up in a separate issue

Agree.

I'm opening two issues to track them: #5021 and #5022

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 10, 2019

Is this actually hotfixed? If so, we have a conflict, which seems bad. We should get it merged ASAP.

@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Jan 22, 2019

This is live on .com yes, there was a bug causing builds to fail with a generic error.

@agjohnson agjohnson merged commit af867c1 into master Jan 22, 2019
1 check passed
@agjohnson agjohnson deleted the hotfix/multipart-api-command branch Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants