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

Add timeout support on upload operations #226

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add timeout support on upload operations #226

wants to merge 2 commits into from

Conversation

greenwolffr
Copy link

@greenwolffr greenwolffr commented May 8, 2020

Adds the support for timeout on upload request.

According to this old post:
https://groups.google.com/a/shotgunsoftware.com/d/msg/shotgun-dev/P-YXTWs9mp0/tzXtkIALt1sJ

It was not added to shotgun API due to a backward compatibility necessity with python 2.4.
Since python 2.6 is now a requirement for the API to work it does not create an issue anymore and can be added.

It has become a priority due to the COVID situation, Shotgun creates a lot of timeouts when uploading thumbnails and playblasts.

@greenwolffr
Copy link
Author

I am waiting for some test, but I will also add a SSLError except to raise a ShotgunError instead of a Fault.

@jfboismenu
Copy link
Contributor

Hi @greenwolffr , what kinds of timeouts are you seeing exactly? It is when uploading large or small files? If there a specific time one day this is happening?

@greenwolffr
Copy link
Author

Hi @jfboismenu , we are experiencing timeouts when uploading medium playblast video files size (~3MB).

This happens every day like 10-20 times a day according to our wranglers, mostly during business time when there is a lot of traffics. We already confirmed that it's due to an HTTP timeout issue, but since upload does not have time out configured, the request never ends and scripts hangs forever.

This patch fixed the issue on our side, with this modification we can now detect timeouts and retry the request.

@jfboismenu
Copy link
Contributor

Hi!
Alright, I'll discuss this with the team, this sounds totally fair!

@jfboismenu
Copy link
Contributor

jfboismenu commented Jun 3, 2020

Hi again, we're quite busy at the moment and we want to properly evaluate the impact and QA your fix under different conditions. We're going to look into how widespread this issue is for our clients at the moment, which will raise or lower the priority for us to merge this.

If I'm not wrong everything is under control on your end now that you have this fix, correct?

We'll keep you posted.

@pscadding pscadding added the Logged logged to jira label Jun 4, 2020
@greenwolffr
Copy link
Author

Hi, thanks for considering the request.

Yes for now things seems to be in control on our side. The only pitfall we had for now, is that having such a patch make difficult for us to update safely the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Logged logged to jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants