uranusjr commented on code in PR #76571:
URL: https://github.com/apache/airflow/pull/76571#discussion_r4510097555
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -1181,8 +1184,34 @@ def __init__(self, client: Client):
self.client = client
def run(self, callback_id: uuid.UUID) -> None:
- """Exchange the single-use callback token for an execution token."""
- self.client.patch(f"callbacks/{callback_id}/run")
+ """
+ Exchange the single-use callback token for an execution token.
+
+ The server uses up the token once it gets the request, so only retry
when the request never
+ reached it, and give a slow server longer to answer.
+ """
+ try:
Review Comment:
An idempotency key would keep single-use semantics. The worker sends a
per-attempt nonce with the exchange, and the server re-issues only for a
matching nonce. A copy of the broker message doesn't have it. Fine with this as
a 3.4.1 stopgap, but please file an issue for that follow-up and link it here.
On 502/503, not knowing whether the token was used isn't a reason not to
retry. If it was used, the retry gets a 409 and the callback fails, the same as
now. If it wasn't (a 503 from an LB with no upstream, or a 500 that rolled
back), the retry succeeds. So `retry=False` only changes the error message, and
it costs the restart case this PR is meant to fix.
Can we keep `_should_retry_api_request` and only extend the timeout and the
retry budget? The "outcome unknown" log can move to a 409 that follows an
earlier failed attempt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
