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.

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

get_task_instance_try_details API returns TaskInstanceHistory schema #43830

Merged
merged 12 commits into from
Nov 18, 2024

Conversation

kandharvishnu
Copy link
Contributor

@kandharvishnu kandharvishnu commented Nov 8, 2024

these API requests

  • get_task_instance_try_details
  • get_mapped_task_instance_try_details
  • get_task_instance_tries
  • get_mapped_task_instance_tries

are actually returning TaskInstanceHistory


Airflow API Docs mentions that the get_task_instance_try_details return task_instance schema
image

But API actually returns TaskInstanceHistory schema

image

get_task_instance_try_details method also returns the TaskInstanceHistory

image

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries
 
are actually returning TaskInstanceHistory
@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Nov 8, 2024
@rawwar rawwar added the legacy api Whether legacy API changes should be allowed in PR label Nov 8, 2024
@rawwar rawwar added the legacy ui Whether legacy UI change should be allowed in PR label Nov 12, 2024
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside the small comment, looking good.

Maybe @ephraimbuddy can double check ?

@ephraimbuddy
Copy link
Contributor

There's no difference between TI and TI History except for loading related objects. It makes me wonder if we need this PR

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 12, 2024

There's no difference between TI and TI History except for loading related objects. It makes me wonder if we need this PR

In that case exposing a TaskInstance might be sufficient and less confusing for users of the API. Otherwise they might be intrigued by TaskInstanceHistory. Maybe in this PR we can just add a comment in the code, in the TaskInstanceHistory Schema to specify that there are no differences beside loading objects, and that in terms of response TaskInstance is fine ? (All that will be deleted soon anyway).

Also there is the deprecated execution_date difference, maybe the TaskInstanceHistory is missing the logical_date/execution_date ?

@kandharvishnu
Copy link
Contributor Author

Also there is the deprecated execution_date difference, maybe the TaskInstanceHistory is missing the logical_date/execution_date ?

Additionally, I noticed that the following columns are missing in TaskInstanceHistory when compared with TaskInstance:

  • sla_miss
  • rendered_map_index
  • rendered_fields
  • trigger
  • triggerer_job
  • note

@ephraimbuddy
Copy link
Contributor

Also there is the deprecated execution_date difference, maybe the TaskInstanceHistory is missing the logical_date/execution_date ?

the logical_date/execution_date is a proxy in TaskInstance so we couldn't have it in TI history.

@ephraimbuddy
Copy link
Contributor

Also there is the deprecated execution_date difference, maybe the TaskInstanceHistory is missing the logical_date/execution_date ?

Additionally, I noticed that the following columns are missing in TaskInstanceHistory when compared with TaskInstance:

  • sla_miss
  • rendered_map_index
  • rendered_fields
  • trigger
  • triggerer_job
  • note

Those are the differences. We can't be able to have those in TI history.

@pierrejeambrun
Copy link
Member

Those are the differences. We can't be able to have those in TI history.

As there are some differences I think we need to actually fix the spec with the actual response returned. Otherwise client might expect one of those fields trigger, etc.. while those are missing.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend to wait for @ephraimbuddy second opinion before merging that. But looks good to me.

@ephraimbuddy ephraimbuddy merged commit 6f02fdb into apache:main Nov 18, 2024
55 checks passed
pierrejeambrun pushed a commit to astronomer/airflow that referenced this pull request Nov 18, 2024
…pache#43830)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries

are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
(cherry picked from commit 6f02fdb)
pierrejeambrun added a commit that referenced this pull request Nov 18, 2024
…43830) (#44133)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries

are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
(cherry picked from commit 6f02fdb)

Co-authored-by: kandharvishnu <46064835+kandharvishnu@users.noreply.github.com>
amoghrajesh pushed a commit to astronomer/airflow that referenced this pull request Nov 18, 2024
…pache#43830)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries
 
are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
kandharvishnu added a commit to kandharvishnu/airflow that referenced this pull request Nov 19, 2024
…pache#43830)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries
 
are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
utkarsharma2 pushed a commit that referenced this pull request Dec 4, 2024
…43830) (#44133)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries

are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
(cherry picked from commit 6f02fdb)

Co-authored-by: kandharvishnu <46064835+kandharvishnu@users.noreply.github.com>
utkarsharma2 pushed a commit that referenced this pull request Dec 9, 2024
…43830) (#44133)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries

are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
(cherry picked from commit 6f02fdb)

Co-authored-by: kandharvishnu <46064835+kandharvishnu@users.noreply.github.com>
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
…pache#43830)

* Update v1.yaml

these
- get_task_instance_try_details
- get_mapped_task_instance_try_details
- get_task_instance_tries
- get_mapped_task_instance_tries
 
are actually returning TaskInstanceHistory

* Update v1.yaml

* dummy change

* revert "dummy change"

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* Update api-generated.ts

* changes to v1.yaml

* Update api-generated.ts

* removing execution_date

---------

Co-authored-by: kandharvishnuu <148410552+kandharvishnuu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API legacy api Whether legacy API changes should be allowed in PR legacy ui Whether legacy UI change should be allowed in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants