Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Port LocalTaskJob tests to Task SDK #44356

Closed
kaxil opened this issue Nov 25, 2024 · 3 comments · Fixed by #44590
Closed

Port LocalTaskJob tests to Task SDK #44356

kaxil opened this issue Nov 25, 2024 · 3 comments · Fixed by #44590
Assignees
@kaxil
Labels
area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK

Comments

@kaxil
Copy link
Member

kaxil commented Nov 25, 2024 •
edited
Loading

To get more confidence in the new Supervisor process in the Task SDK, we should investigate and port over tests from LocalTaskJob and some from models.TI to Task SDK's superisor's test
@kaxil kaxil self-assigned this Nov 25, 2024
@kaxil kaxil converted this from a draft issue Nov 25, 2024
@kaxil kaxil added the area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK label Nov 25, 2024
@kaxil
Copy link
Member Author

kaxil commented Nov 26, 2024

WIP: https://github.com/apache/airflow/compare/main...astronomer:airflow:ltj-tests?expand=1

kaxil added a commit to astronomer/airflow that referenced this issue Nov 27, 2024
@kaxil
AIP-72: Improve Supervisor and Task Instance State Validation
6256ddc 
This commit adds test that Supervisor prevents starting a Task Instance that is already running. Also updated `test_ti_update_state_conflict_if_not_queued` to use parameterized states, covering all non-queued Task Instance states.

This is part of my efforts to port LocalTaskJob tests to Supervisor: apache#44356.

This ports over `TestLocalTaskJob.test_localtaskjob_double_trigger`
This was referenced Nov 27, 2024
Merged
Merged
kaxil added a commit to astronomer/airflow that referenced this issue Nov 28, 2024
@kaxil
AIP-72: Handle External update TI state in Supervisor
b7ea7f4 
- Updated logic to handle externally updated TI state in Supervisor. This states could have been externally changed via UI, CLI, API etc
- Replaced `FASTEST_HEARTBEAT_INTERVAL` and `SLOWEST_HEARTBEAT_INTERVAL` with `MIN_HEARTBEAT_INTERVAL` and `HEARTBEAT_THRESHOLD` for better clarity

This is part of my efforts to port LocalTaskJob tests to Supervisor: apache#44356.

This ports over `TestLocalTaskJob.test_mark_{success,failure}_no_kill`.

This PR also allows retrying heartbeats:

- Added `_last_successful_heartbeat` and `_last_heartbeat_attempt` for better separation of tracking successful heartbeats and retries.
- `MIN_HEARTBEAT_INTERVAL` is now respected between heartbeat attempts, even after failures.
-  The num of retries is configurable via `MAX_FAILED_HEARTBEATS`
kaxil added a commit to astronomer/airflow that referenced this issue Nov 28, 2024
@kaxil
AIP-72: Handle External update TI state in Supervisor
0893557 
- Updated logic to handle externally updated TI state in Supervisor. This states could have been externally changed via UI, CLI, API etc
- Replaced `FASTEST_HEARTBEAT_INTERVAL` and `SLOWEST_HEARTBEAT_INTERVAL` with `MIN_HEARTBEAT_INTERVAL` and `HEARTBEAT_THRESHOLD` for better clarity

This is part of my efforts to port LocalTaskJob tests to Supervisor: apache#44356.

This ports over `TestLocalTaskJob.test_mark_{success,failure}_no_kill`.

This PR also allows retrying heartbeats:

- Added `_last_successful_heartbeat` and `_last_heartbeat_attempt` for better separation of tracking successful heartbeats and retries.
- `MIN_HEARTBEAT_INTERVAL` is now respected between heartbeat attempts, even after failures.
-  The num of retries is configurable via `MAX_FAILED_HEARTBEATS`
kaxil added a commit to astronomer/airflow that referenced this issue Nov 28, 2024
@kaxil
AIP-72: Handle External update TI state in Supervisor
560b961 
- Updated logic to handle externally updated TI state in Supervisor. This states could have been externally changed via UI, CLI, API etc
- Replaced `FASTEST_HEARTBEAT_INTERVAL` and `SLOWEST_HEARTBEAT_INTERVAL` with `MIN_HEARTBEAT_INTERVAL` and `HEARTBEAT_THRESHOLD` for better clarity

This is part of my efforts to port LocalTaskJob tests to Supervisor: apache#44356.

This ports over `TestLocalTaskJob.test_mark_{success,failure}_no_kill`.

This PR also allows retrying heartbeats:

- Added `_last_successful_heartbeat` and `_last_heartbeat_attempt` for better separation of tracking successful heartbeats and retries.
- `MIN_HEARTBEAT_INTERVAL` is now respected between heartbeat attempts, even after failures.
-  The num of retries is configurable via `MAX_FAILED_HEARTBEATS`
kaxil added a commit that referenced this issue Nov 28, 2024
@kaxil
AIP-72: Handle External update TI state in Supervisor (#44406)
6e3a25e 
- Updated logic to handle externally updated TI state in Supervisor. This states could have been externally changed via UI, CLI, API etc
- Replaced `FASTEST_HEARTBEAT_INTERVAL` and `SLOWEST_HEARTBEAT_INTERVAL` with `MIN_HEARTBEAT_INTERVAL` and `HEARTBEAT_THRESHOLD` for better clarity

This is part of my efforts to port LocalTaskJob tests to Supervisor: #44356.

This ports over `TestLocalTaskJob.test_mark_{success,failure}_no_kill`.

This PR also allows retrying heartbeats:

- Added `_last_successful_heartbeat` and `_last_heartbeat_attempt` for better separation of tracking successful heartbeats and retries.
- `MIN_HEARTBEAT_INTERVAL` is now respected between heartbeat attempts, even after failures.
-  The num of retries is configurable via `MAX_FAILED_HEARTBEATS`
@kaxil kaxil mentioned this issue Nov 29, 2024
Merged
@kaxil
Copy link
Member Author

kaxil commented Nov 29, 2024

#44405 & #44405 got merged

PRs:

@kaxil
Copy link
Member Author

kaxil commented Nov 30, 2024

Next Up:

kaxil added a commit that referenced this issue Nov 30, 2024
@kaxil
AIP-72: Add escalation path for killing process in Supervisor (#44465)
0c03f03 
Added logic to support escalation from SIGINT to SIGTERM and SIGKILL when killing a task process from the new Supervisor process.

part of #44356
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
48ca10c 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
@kaxil kaxil mentioned this issue Dec 3, 2024
Merged
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
72ed26f 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
f0ce4de 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
da50ce0 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
7864d5c 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
1d0afab 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
4be85f7 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
752e4f3 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit to astronomer/airflow that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor
5b8fd67 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
kaxil added a commit that referenced this issue Dec 3, 2024
@kaxil
AIP-72: Port task success overtime to the Supervisor (#44590)
d059d4a 
This PR ports the overtime feature on `LocalTaskJob` (added in #39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes #44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this issue Jan 5, 2025
@kaxil
AIP-72: Handle External update TI state in Supervisor (apache#44406)
00d1ebf 
- Updated logic to handle externally updated TI state in Supervisor. This states could have been externally changed via UI, CLI, API etc
- Replaced `FASTEST_HEARTBEAT_INTERVAL` and `SLOWEST_HEARTBEAT_INTERVAL` with `MIN_HEARTBEAT_INTERVAL` and `HEARTBEAT_THRESHOLD` for better clarity

This is part of my efforts to port LocalTaskJob tests to Supervisor: apache#44356.

This ports over `TestLocalTaskJob.test_mark_{success,failure}_no_kill`.

This PR also allows retrying heartbeats:

- Added `_last_successful_heartbeat` and `_last_heartbeat_attempt` for better separation of tracking successful heartbeats and retries.
- `MIN_HEARTBEAT_INTERVAL` is now respected between heartbeat attempts, even after failures.
-  The num of retries is configurable via `MAX_FAILED_HEARTBEATS`
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this issue Jan 5, 2025
@kaxil
AIP-72: Add escalation path for killing process in Supervisor (apache…
bac8a14 
…#44465)

Added logic to support escalation from SIGINT to SIGTERM and SIGKILL when killing a task process from the new Supervisor process.

part of apache#44356
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this issue Jan 5, 2025
@kaxil
AIP-72: Port task success overtime to the Supervisor (apache#44590)
3cc1994 
This PR ports the overtime feature on `LocalTaskJob` (added in apache#39890) to the Supervisor.
It allows to terminate Task process to terminate when it exceeding the configured success overtime threshold which is useful when we add Listenener to the Task process.

closes apache#44356

Also added `TaskState` to update state and send end_date from task process to the supervisor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kaxil kaxil

Labels
area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK
Projects
AIP-72 - Task Execution Interface and SDK
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

AIP-72: Port task success overtime to the Supervisor astronomer/airflow
1 participant
@kaxil