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

Displaying "actual" try number in TaskInstance view #34635

Merged

Conversation

SamWheating
Copy link
Contributor

closes: #34309

Since we do some weird stuff with the try_number property, we have to do some weird stuff in order to properly search / display it in the UI 🥴:

  1. Overriding try_number.expression so that sqlalchemy only sees the actual try_number value when searching or filtering.
  2. Swapping in the prev_attempted_tries property in place of the try_number property in order to display the actual try_number value.

This causes the UI to function as expect, only showing + filtering on the actual value from the DB:
image

However, this feels pretty hacky and I think that overriding the hybrid_property expression in particular might cause some issues down the line. I am going to see how complicated it would be to remove the weird try_number property altogether, moving the try_number+1 logic to a separate property or selectively incrementing the value where required.

All feedback and suggestions welcome.

@potiuk potiuk force-pushed the sw-fix-try-number-in-taskinstance-view branch from 9258a24 to 4081e24 Compare October 8, 2023 19:21
@potiuk potiuk force-pushed the sw-fix-try-number-in-taskinstance-view branch from 4081e24 to 3d60e85 Compare October 15, 2023 21:09
@potiuk
Copy link
Member

potiuk commented Oct 21, 2023

docs need fixing for that one (and rebase).

@eladkal eladkal added this to the Airflow 2.7.3 milestone Oct 27, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Oct 27, 2023
@eladkal
Copy link
Contributor

eladkal commented Oct 27, 2023

ping @SamWheating there is still time for this fix to be included in next release

@SamWheating
Copy link
Contributor Author

Thanks for the bump, looking into this now 👀

How do you feel about the approach in general? It feels kinda hacky to me, but it technically works so 🤷

@SamWheating SamWheating force-pushed the sw-fix-try-number-in-taskinstance-view branch from 3d60e85 to c3a7aaa Compare November 2, 2023 00:04
@SamWheating SamWheating force-pushed the sw-fix-try-number-in-taskinstance-view branch from c3a7aaa to b47d3bd Compare November 24, 2023 00:46
@eladkal
Copy link
Contributor

eladkal commented Nov 24, 2023

How do you feel about the approach in general? It feels kinda hacky to me, but it technically works so

@bbovenzi approved so I guess it's OK :)

@bbovenzi
Copy link
Contributor

bbovenzi commented Dec 6, 2023

How do you feel about the approach in general? It feels kinda hacky to me, but it technically works so

@bbovenzi approved so I guess it's OK :)

Yeah, if this gives the correct number then I'm happy with it. We should check that everywhere uses the right try_number.

@eladkal
Copy link
Contributor

eladkal commented Jan 10, 2024

@bbovenzi can we merge it then?

@eladkal eladkal force-pushed the sw-fix-try-number-in-taskinstance-view branch from f370ca4 to e8858b9 Compare January 10, 2024 10:16
@potiuk potiuk merged commit ce6ac41 into apache:main Jan 31, 2024
53 checks passed
@potiuk
Copy link
Member

potiuk commented Jan 31, 2024

Merging :)

jedcunningham pushed a commit that referenced this pull request Feb 9, 2024
potiuk pushed a commit that referenced this pull request Feb 13, 2024
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
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.

Incorrect try_number when searching task instances
5 participants