Skip to content

Go SDK: send execution_timeout and use the worker's default - #76359

Open
ChickenBenny wants to merge 3 commits into
apache:mainfrom
ChickenBenny:go-sdk-execution-timeout
Open

ChickenBenny wants to merge 3 commits into
apache:mainfrom
ChickenBenny:go-sdk-execution-timeout

Conversation

@ChickenBenny

Copy link
Copy Markdown
Contributor

Follow-up 2 of #75387.

After #75387 the supervisor starts the execution_timeout clock for a language SDK task as soon as the runtime has the task, using TIRunContext.execution_timeout. A Python task has no clock until it sends SetExecutionTimeout right before it runs. The Go runtime never sent it, which causes two problems:

  1. A Go task that sets no execution_timeout doesn't get the worker's [core] default_task_execution_timeout. Right now it gets the Dag processor's default through the serialized Dag, but a follow-up will stop storing that default there, and without this change the task would then have no timeout at all.
  2. The clock starts a bit earlier than for a Python task.

Changes

  • When ti_context.execution_timeout is unset, the coordinator fills it in with the worker's [core] default_task_execution_timeout, the same value a Python task gets when the worker parses its Dag. It starts the supervisor's clock with that value and sends it to the runtime in StartupDetails. A 0 means no timeout, so it doesn't fall back. Each runtime gets the default without reading any config itself: the TypeScript runtime, and a runtime that never sends SetExecutionTimeout, get it too.

  • Right before the task runs (after XComs are cleared), the Go runtime reads the timeout from ti_context and does what the Python task runner does:

    • not set, or 0: sends nothing (no timeout).
    • positive: sends SetExecutionTimeout, which restarts the supervisor's clock from that point.
    • negative: fails the task without running it.

    A failed task retries if should_retry is set.

  • The Go runtime still doesn't stop a timed-out task by itself. The supervisor sends SIGTERM after the timeout plus killed_task_cleanup_time, same as before. Stopping the task at the timeout is the Go part of Language SDK runtimes do not handle execution_timeout in the task process #74717 and will be a separate PR.

  • Separate commit: the iface* helpers move from messages.go to iface.go, and ifaceInt and the new ifaceFloat share a generic ifaceNumber. No behavior change.

What's next

A follow-up in core and the standard provider, opened as a draft and merged after this PR and #76103, will change what the serialized Dag stores for a stub task's execution_timeout:

Stub task sets Stored Timeout used
nothing (default_args counts as set) nothing the worker default, filled in by the coordinator
None 0 none
a timedelta its seconds (same as today) that value

It will also drop execution_timeout from client_defaults. Until then, a stub task with None is treated the same as one that sets nothing. That follow-up has to go in last because it removes the default that language SDK tasks get today.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:coordinator Coordinator: The interface to spawn Lang-SDK subprocesses area:go-sdk area:task-sdk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant