Move TaskInstance heartbeat directly on to TI row, not on Job row by ashb · Pull Request #43599 · apache/airflow

ashb · 2024-11-01T18:50:00Z

This is part of the work for AIP-72 epic, but is done as a separate PR for
ease of review.

This PR byitself doesn't remove the LocalTaskJob row (that will happen in a
future PR when the exeuction code is moved over to live in the TaskSDK) but
this paves the way for it. The reason we are making this change is:

Having a separate row for tracking TI heartbeat is not really buying us much
With the addition of TaskInstanceHistory we don't need another separate
record of when/where TIs were run
It simplifies things (one less join in finding zombies)
Makes zombie tracking eaiser -- it is now just on the TI state, not the
combination of TI and Job state.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

This is part of the work for AIP-72 epic, but is done as a separate PR for ease of review. This PR byitself doesn't remove the LocalTaskJob row (that will happen in a future PR when the exeuction code is moved over to live in the TaskSDK) but this paves the way for it. The reason we are making this change is: - Having a separate row for tracking TI heartbeat is not really buying us much - With the addition of TaskInstanceHistory we don't need _another_ separate record of when/where TIs were run - It simplifies things (one less join in finding zombies) - Makes zombie tracking eaiser -- it is now just on the TI state, not the combination of TI and Job state.

dstandish

some suggestions

jscheffl

Never noticed this structure and even w/o AIP-72 this seems to be odd. Good that you move it.

@dstandish

Follow-up of apache#43599, specifically https://github.com/apache/airflow/pull/43599/files#r1826351853 from @dstandish.

…ache#43599) This is part of the work for AIP-72 epic, but is done as a separate PR for ease of review. This PR by itself doesn't remove the LocalTaskJob row (that will happen in a future PR when the execution code is moved over to live in the TaskSDK) but this paves the way for it. The reason we are making this change is: - Having a separate row for tracking TI heartbeat is not really buying us much - With the addition of TaskInstanceHistory we don't need _another_ separate record of when/where TIs were run - It simplifies things (one less join in finding zombies) - Makes zombie tracking easier -- it is now just on the TI state, not the combination of TI and Job state.

ashb requested review from XD-DENG, bolkedebruin, hussein-awala, o-nikolas, pierrejeambrun and potiuk as code owners November 1, 2024 18:50

boring-cyborgBot added area:CLI area:db-migrations PRs with DB migration area:dev-tools area:Executors-core LocalExecutor & SequentialExecutor area:Scheduler including HA (high availability) scheduler area:serialization kind:documentation labels Nov 1, 2024

jedcunningham approved these changes Nov 1, 2024
View reviewed changes