Databricks hook - retry on HTTP Status 429 as well

[alexott]

If Databricks control plane receives too many API requests it starts to return HTTP code 429, and caller should retry that request. But Databricks hook retried only on 5xx status code.

closes: #21559

[potiuk]

Don't you think this should have exponential back-off on 429 (at the very least but also 500 IMHO). This is highly likely that you will only increase the problem with retrying with fixed retry_delay. We use tenacity for similar cases in other places - so maybe you should change it too @alexott to follow the same pattern ?

[alexott]

Yes, I thought about it as well, but I’ll need to think again. Let me mark this PR as draft

[potiuk]

Look for tenacity in google providers :)

[potiuk]

Or HTTP or SFTP. Same pattern was used there in a number of places.

[uranusjr]

If I am the API endpoint’s maintainer, I would ver much prefer if a client does not retry if I tell them 429, at least not before my specified Retry-After. 429 tells you to stop, not to try harder.

[potiuk]

If I am the API endpoint’s maintainer, I would ver much prefer if a client does not retry if I tell them 429, at least not before my specified Retry-After. 429 tells you to stop, not to try harder.

Exponential backoff we use for that is actually the best of both worlds. It does not stop, but it also decreases the pressure.

[uranusjr]

Exponential backoff still tries too early in most situations. A client receiving 429 is supposed to wait at least until the date specified in the Retry-After response header before any retries.

[potiuk]

Exponential backoff still tries too early in most situations. A client receiving 429 is supposed to wait at least until the date specified in the Retry-After response header before any retries.

Sure. Retry-After should be the source of first retry time - but exponential back-off after that does not hurt.

[potiuk]

Exponential backoff still tries too early in most situations. A client receiving 429 is supposed to wait at least until the date specified in the Retry-After response header before any retries.

Sure. Retry-After should be the source of first retry time - but exponential back-off after that does not hurt.

Just to add a bit more reasoning (I thought a bit about it).

The problem is that Retry-After is only a hint, not a "source of truth". It relies on the fact the server "knows" what it is doing. Which is not necessary valid:
a) it might be based on past information (which might be outdated and might not include the mounting spike traffic properly) - this happens often
b) it might be simply not there. often you will not get 429 but 5XX in similar situations because it's not only the server that gets flooded but also some gateways on the way or simply the server might timeout or run out of memory or other resources.

So IMHO it needs to be client to decide how to behave. One added value of exponential backoff is that it is still helpful in all retriable conditions that are "unknown" - i.e. 5XX. Those do not contain "Retry-After" to base your decision on.

So I think exponential back-off with some initial timeout (if Retry-After is available - it should be a starting point) should be the right approach. Additionally if even including exponential back-off, you get 429 with Retry-After where your exponential back-off next step is not long enough - the timeout should be re-adjusted to the "Retry-After" received. But if it is longer, then we should continue exponential back-off (because server information might be already out-dated and not include mounting traffic spike).

[alexott]

@potiuk I think that it's ready for review now...

[potiuk]

LGTM. @uranusjr ?

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.