SG-11528 Add optional sleep between retries #196
Conversation
sleep between request attempts for the configured time, default to 3 seconds, and allow override through optional environment variable.
shotgun_api3/shotgun.py
Outdated
| # The interval between _call_rpc retries. Since the Config can't be | ||
| # modified until the Shotgun instance is created, use an environment | ||
| # variable to override the default value. | ||
| self.rpc_attempt_interval = os.environ.get("SHOTGUN_API_RETRY_INTERVAL", 3) |
There was a problem hiding this comment.
Could this also be an optional argument when a Shotgun instance is initialized? I'm thinking it would work like the ca_certs argument, where it can be passed in OR set via an env var. You can store the retry sleep duration as a property on the Shotgun object and _Config can read it from there.
That has the added benefit of giving us a good place to document this and the environment variable.
There was a problem hiding this comment.
Also, however this is documented, it would be good to specifically mention that it's expecting to receive a value in seconds.
There was a problem hiding this comment.
I think there’s a bug with your env var. If it is set, you’ll get a string back, not an int. You’ll need to convert the value before using it
May I suggest having the evr var be in milliseconds instead of seconds? There sort of time settings are usually in milliseconds not seconds.
There was a problem hiding this comment.
I added a kwarg to Shotgun's init, and I'm now storing the value on the _Config instance. I kept the rpc_attempt_interval property on _Config, since this seemed consistent with other similar properties (notably max_rpc_attempts), but I'm happy to move it to the Shotgun object if you think that would be cleaner.
Added a cast to int when getting the env var value, and am now storing wait in ms.
jfboismenu
left a comment
jfboismenu
left a comment
There was a problem hiding this comment.
Looks good, but could you fix the env var and add a test that tests said env var? Thanks!
shotgun_api3/shotgun.py
Outdated
| # The interval between _call_rpc retries. Since the Config can't be | ||
| # modified until the Shotgun instance is created, use an environment | ||
| # variable to override the default value. | ||
| self.rpc_attempt_interval = os.environ.get("SHOTGUN_API_RETRY_INTERVAL", 3) |
There was a problem hiding this comment.
I think there’s a bug with your env var. If it is set, you’ll get a string back, not an int. You’ll need to convert the value before using it
May I suggest having the evr var be in milliseconds instead of seconds? There sort of time settings are usually in milliseconds not seconds.
store interval in milliseconds, add tests for both environment variable and parameter.
| if attempt == max_rpc_attempts: | ||
| raise | ||
| except Exception: | ||
| #TODO: LOG ? |
There was a problem hiding this comment.
It's not your code, but I'm really worried about this except clause. Any errors raises by the code, even programming errors on our part, will retry the request. This seems dangerous to me.
@thebeeland , should we log a tech debt ticket about this and figure out how to properly fix it? We'd have to take into account all the different http and socket error types that can be encountered here.
There was a problem hiding this comment.
We should, and I will. Thanks for the heads up.
jfboismenu
left a comment
jfboismenu
left a comment
There was a problem hiding this comment.
Hi Will,
The code looks good, but there's a bug in your tests. Once this is fixed you're good to go!
manneohrstrom
left a comment
manneohrstrom
left a comment
There was a problem hiding this comment.
can this be a property on the API instance rather than a constructor param?
to prevent leaking SHOTGUN_API_RETRY_INTERVAL env var in the event that tests fail.
explaining config.rpc_attempt_interval property.
manneohrstrom
left a comment
manneohrstrom
left a comment
There was a problem hiding this comment.
Looks great! Apologies for the delay @willis102 !
One question/suggestion - I think we should take this opportunity and document the environment variables used by the Shotgun API. Add a section to the API docs where we summarize them. Either as part of this work, or we should add a ticket to do it!
No more review needed! 💯😄👍
in the SHOTGUN_API_RETRY_INTERVAL environment variable.
Add optional sleep between request attempts defaulting to 3 seconds, and allow override through optional environment variable.