False 500 testing #57
Conversation
- we don't need to force a retry on a 409, it will only result in another 409, so let's just directly handle the issue - try_history back to 500
jonathanfeng-scale
requested review from
fatihkurtoglu,
shaun-scale and
kevin-xu-scale
scaleapi/api.py
Outdated
| @@ -109,9 +109,28 @@ def _api_request( | |||
| json = None | |||
| if res.status_code == 200: | |||
| json = res.json() | |||
| elif res.status_code == 409: | |||
There was a problem hiding this comment.
I think our API also returns 409 (Conflict) during batch and project creation.
Probably super rarer to have a 500 during batch or project creation, but should we add further governance here to ensure that behavior applies only to task creation?
Checking the task/ included in endpoint in a POST request and checking if unique_id is available in the body can be some ways to approach that.
There was a problem hiding this comment.
good idea, will do that
scaleapi/api.py
Outdated
| # See if the first retry was a 500 error | ||
| if retry_history[0][3] == 500: | ||
| uuid = body["unique_id"] | ||
| newUrl = f"https://api.scale.com/v1/tasks?unique_id={uuid}" |
There was a problem hiding this comment.
| newUrl = f"https://api.scale.com/v1/tasks?unique_id={uuid}" | |
| newUrl = f"{self.base_api_url}/tasks?unique_id={uuid}" |
There was a problem hiding this comment.
Probably would be ideal later if we have an API method that directly returns a single task for the given unique_id (similar to getting a task with task id)
There was a problem hiding this comment.
yeah that would be ideal
| newRes = self._http_request( | ||
| "GET", newUrl, headers=headers, auth=auth | ||
| ) | ||
| json = newRes.json()["docs"][0] |
There was a problem hiding this comment.
To confirm, will this json be the same as what we return after task creation, right?
| # redirect_location=None) | ||
| if retry_history != (): | ||
| # See if the first retry was a 500 error | ||
| if retry_history[0][3] == 500: |
There was a problem hiding this comment.
Just to confirm (since I'm not super sure) but is 500 the only error returned from our API for those race conditions? (and not other 5xx errors).
There was a problem hiding this comment.
Yeah, the exact error is task creation taking longer than our timeout, so Scale sends a 500 back in the response
|
@jonathanfeng-scale Also, let's bump the version too |
Here's the test on Python that's run with Pytest:
Here's the console.logs from the local-machine where I'm "reproducing" a similar error (returns 500, but successfully creates task):
So, the test for this was kinda weird, I made our task creation handler force an error during the first task creation. This had to be forced after the task has been created, but before the 200 is sent back.
The same error can't happen on the second task creation, but it should fail in a 409