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

Fix try_number shown in the task?task_id=&dag_id=&execution_date= #32361

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

Adaverse
Copy link
Contributor

@Adaverse Adaverse commented Jul 5, 2023

Closes: #32290
Details - #32290 (comment)

Fixes the try number shown in the tasks view which are tasks that have reached terminal states. currently, it has a value one more than the actual one. The field containing the correct value is _try_number.


^ 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.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jul 5, 2023
@potiuk
Copy link
Member

potiuk commented Jul 5, 2023

I am not 100% sure if this is right in all cases - maybe other can confirm, but regardless, this changes the output of the public API, which someone might rely on (even if it was wrong). If we agree to change it (and classify it as bugfix, not a breaking change), then at the very least, we should have a significant newsfragment describing the change.

@Adaverse
Copy link
Contributor Author

Adaverse commented Jul 5, 2023

I am not 100% sure if this is right in all cases - maybe other can confirm, but regardless, this changes the output of the public API, which someone might rely on (even if it was wrong). If we agree to change it (and classify it as bugfix, not a breaking change), then at the very least, we should have a significant newsfragment describing the change.

From public API I understand the endpoint (correct me if I am wrong here) - api/v1/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id} and the value of try_number here is already coming from _try_number. Do you mean the same when you say public API?

The API I have changed returns the HTML page. I think its safe to fix this without any impact to users. It in fact syncs with the public API after this fix.

@potiuk
Copy link
Member

potiuk commented Jul 5, 2023

I am not 100% sure if this is right in all cases - maybe other can confirm, but regardless, this changes the output of the public API, which someone might rely on (even if it was wrong). If we agree to change it (and classify it as bugfix, not a breaking change), then at the very least, we should have a significant newsfragment describing the change.

From public API I understand the endpoint (correct me if I am wrong here) - api/v1/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id} and the value of try_number here is already coming from _try_number. Do you mean the same when you say public API?

The API I have changed returns the HTML page. I think its safe to fix this without any impact to users. It in fact syncs with the public API after this fix.

Ah right stupid me (note to self: read more carefully) :). 🤦 , sorry.

Indeed - I have looked mostly at the description, not the code when I made the assessment.

The ones in the public API come from the table - which might indeed be different from the TaskInstance try_num property as you mentioned in the linked issue.

I wonder if there is a case it might be wrong (I don't think so- looks like it will always be "right" looking at the property.

@potiuk potiuk merged commit a8e4b8a into apache:main Jul 5, 2023
42 checks passed
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Jul 6, 2023
@ephraimbuddy ephraimbuddy added this to the Airlfow 2.6.3 milestone Jul 6, 2023
ephraimbuddy pushed a commit that referenced this pull request Jul 6, 2023
…32361)

* try_number fix

* change test name

(cherry picked from commit a8e4b8a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try number is incorrect
3 participants