seanghaeli commented on code in PR #76571:
URL: https://github.com/apache/airflow/pull/76571#discussion_r4533922231
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -1229,6 +1268,8 @@ def noop_handler(request: httpx2.Request) ->
httpx2.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
API_SSL_CA_FILE_PATH = conf.get("api", "ssl_ca_file", fallback=None)
API_TIMEOUT = conf.getfloat("workers", "execution_api_timeout")
+# The callback token exchange waits for one answer as long as the usual
retries would wait on timeouts.
+API_CALLBACK_RUN_READ_TIMEOUT = API_TIMEOUT * API_RETRIES
Review Comment:
Good catch! I like the max(API_RETRIES,1) angle. Updated to use that
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -1181,8 +1183,45 @@ 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 on the first request it handles, and a
retry after that gets a 409.
+ So give a slow server longer to answer, and keep retrying for longer
while it is down.
+ """
+ reached_server = False
+
+ def before_sleep(retry_state) -> None:
+ # Only an attempt that may have reached the server can have used
the token.
+ nonlocal reached_server
+ if not isinstance(retry_state.outcome.exception(),
(httpx2.ConnectError, httpx2.ConnectTimeout)):
+ reached_server = True
+ _log_and_trace_retry(retry_state)
+
+ retrying = Retrying(
+ retry=retry_if_exception(_should_retry_api_request),
+ # At least the usual number of attempts, and keep going while the
server is down.
+ stop=stop_after_attempt(API_RETRIES) &
stop_after_delay(API_RETRY_WAIT_MAX),
Review Comment:
I'd rather leave the config docs as they are. Anyone changing the
`execution_api_*` settings already knows they apply to every caller of the
Execution API, and within reason won't set them low enough to cause problems.
The callback token exchange is one of those callers, and its time is bounded
from below by the same settings: its read timeout is never shorter than
`execution_api_timeout`, and it keeps retrying at least as long as other calls
do. I'd call it out if we had tied the exchange to settings unrelated to the
Execution API, but here a note about deadline callbacks in the Execution API
docs would be out of place.
--
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]
