Note for reviewers

On 4 August 2022 I reverted all changes in this PR so I could reimplement and apply all your review feedback. This happened infa1fd50. Every subsequent commit encapsulates one logical change to the code. Working through them one-at-a-time should be quite easy.

1. Isolate delay boundingbf65b81
2. Move error details scope up one levela0d340e (H/T@moderakh )
3. RetryGetOperationStatus forOSErrora55cf9d (thanks@sander-goos and@benfleis )
4. Log when we do it38411a8 (H/T@benfleis )

The final four commits are simple code cleanup and fixing one warning in the test suite un-related to this change.

Since our e2e tests are not enabled via Github Actions yet I ran them locally and all passed.

Description

Currently, we only retry attempts that returned a code 429 or 503 and include a Retry-After header. This pull request also allowsGetOperationStatus requests to be retried if the request fails with anOSError exception. The configured retry policy is still honoured with regard to maximum attempts and max retry duration.

Background

The reason we only retry 429 and 503 responses today is because retries must be idempotent. Otherwise the connector could cause unexpected or harmful consequences (data loss, excess resource utilisation etc.)

We know that 429/503 responses are idempotent because the attempt was halted before the server could execute it, regardless if the attempted operation was itself idempotent.

We also know thatGetOperationStatus requests are idempotent because they do not modify data on the server. Therefore we can add an extra case to our retry allow list:

• If a request is aGetOperationStatus command
• If it fails due to anOSError such asTimeoutError orConnectionResetError.

Previously we attempted this same behaviour by retryingGetOperationStatus requests, regardless the nature of the exception. But this change could not pass our e2e tests because there are valid cases whenGetOperationStatus will raise an exception from within our own library code: for example, if an operation is canceled in a separate threadGetOperationStatus will raise a "DOES NOT EXIST" exception.

Logging Behaviour

The connector will log whenever it retries an attempt because of anOSError. It will use log levelINFO if theOSError is one we consider normal. It will use log levelWARNING if theOSError seems unusual. The codes we consider normal are:

# | Debian | Darwin |info_errs= [# |--------|--------|errno.ESHUTDOWN,# |   32   |   32   |errno.EAFNOSUPPORT,# |   97   |   47   |errno.ECONNRESET,# |   104  |   54   |errno.ETIMEDOUT,# |   110  |   60   |                    ]

The full set ofOSError codes is platform specific. I wrote this patch to target a specific customer scenario whenGetOperationStatus requests were retried after an operating system socket timeout exception. In this customer scenario the error code wasErrno 110: Connection timed out. However that error code is specific to Linux. On a Darwin/Macos host the code would be65 and on Windows it would be10060.

Rather than catch these specifically, I use Python'serrno built-in to check forerrno.ETIMEDOUT which will resolve to the platform-specific code at runtime. I tested this manually on Linux and MacOS (but not on Windows).@benfleis helped me pick what we consider "normal".

We log all otherOSError codes withWARNING is because it would be pretty unusual for a request to fail because of a "FileNotFound" or a system fault.

References

I foundthis article extremely helpful while formulating this fix. The author is solving a very similar problem across platforms.