Use DagRun.run_id instead of execution_date when updating state of TIs(UI & REST API)
#18724
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ephraimbuddy
force-pushed
the
add-run-id
branch
2 times, most recently
from
6ff53b8 to
9e5874f
Compare
|
I took the easiest way for this PR. There are a set of functions that uses the execution date underneath this PR but I limited the PR to the REST API. Let me know if those functions need to be touched too |
ephraimbuddy
force-pushed
the
add-run-id
branch
from
9e5874f to
6da4034
Compare
ashb
reviewed
Oct 5, 2021
ashb
reviewed
Oct 5, 2021
ashb
reviewed
Oct 5, 2021
ashb
reviewed
Oct 5, 2021
ephraimbuddy
force-pushed
the
add-run-id
branch
from
6da4034 to
bfc39ea
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
uranusjr
removed
the
stale
ephraimbuddy
force-pushed
the
add-run-id
branch
from
bfc39ea to
5a9ce95
Compare
ephraimbuddy
requested review from
bbovenzi,
ryanahamilton and
XD-DENG
as code owners
uranusjr
reviewed
Nov 22, 2021
ephraimbuddy
force-pushed
the
add-run-id
branch
2 times, most recently
from
6c74ac7 to
fa4ca9e
Compare
ashb
reviewed
Nov 22, 2021
ephraimbuddy
force-pushed
the
add-run-id
branch
from
fa4ca9e to
3f53972
Compare
uranusjr
reviewed
Jan 10, 2022
ephraimbuddy
force-pushed
the
add-run-id
branch
4 times, most recently
from
c7b46d8 to
acd7db4
Compare
uranusjr
reviewed
Jan 13, 2022
airflow/models/dag.py
Outdated
Comment on lines
1671
to
1680
| input_execution_date = execution_date | ||
| if dag_run_id: | ||
| dag_run = ( | ||
| session.query(DagRun).filter(DagRun.run_id == dag_run_id, DagRun.dag_id == self.dag_id).one() | ||
| ) # Raises an error if not found | ||
| resolve_execution_date = dag_run.execution_date |
There was a problem hiding this comment.
How about
if execution_date is None:
dag_run = ...
resolved_execution_date = dag_run.execution_date
else:
resolved_execution_date = execution_dateThis would simplify the later code to
end_date = resolve_execution_date if not future else None
start_date = resolve_execution_date if not future else None
uranusjr
reviewed
Jan 13, 2022
Comment on lines
+332
to
336
| dag_run_id=run_id, | ||
| execution_date=execution_date, |
There was a problem hiding this comment.
This combined with the exactly_one check in set_task_instance_state means the request would error (with 500) if the user passes both execution_date and run_id.=
There was a problem hiding this comment.
We have schema level validation. So it won't allow both values to be supplied
uranusjr
approved these changes
Jan 13, 2022
ashb
reviewed
Jan 13, 2022
| description: | | ||
| The task instance's DAG run ID. Either set this or execution_date but not both. | ||
|
|
||
| *New in version 2.2.3* |
There was a problem hiding this comment.
2.2.3 is already out now isn't it, So 2.2.4 or 2.3.0 (Probably 2.3, as this one doesn't feel like a bug fix so shouldn't be backported.)
ashb
previously requested changes
Jan 13, 2022
airflow/models/taskinstance.py
Outdated
| @@ -742,6 +742,34 @@ def error(self, session=NEW_SESSION): | |||
| session.merge(self) | |||
| session.commit() | |||
|
|
|||
| @classmethod | |||
| @provide_session | |||
| def find( | |||
There was a problem hiding this comment.
This function is only used for this one test, right? If so lets not add it (I'm not sure the pattern we have for DagRun.find etc is good)
There was a problem hiding this comment.
Ah its used one place -- but still, I don't think we need it.
ashb
reviewed
Jan 13, 2022
ephraimbuddy
force-pushed
the
add-run-id
branch
from
216d7ea to
71bc93e
Compare
Error when the task instance does not exist Update airflow/api_connexion/endpoints/task_instance_endpoint.py Co-authored-by: Ash Berlin-Taylor <[email protected]> remove mock use try/accept Support the use of run_id in set_task_instance_state(REST API) We can now use run_id as well as execution_date to update states of task instances deprecate execution_date improve error message add run_id to set_state function require either the run_id or the execution_date fixup! require either the run_id or the execution_date Update airflow/api_connexion/endpoints/task_instance_endpoint.py Co-authored-by: Tzu-ping Chung <[email protected]> fixup! Update airflow/api_connexion/endpoints/task_instance_endpoint.py Ensure supplying both execution_date and run_id raises for dag.set_task_instance_state fixup! Ensure supplying both execution_date and run_id raises for dag.set_task_instance_state Do not depreccate execution_date fixup! Do not depreccate execution_date Use func.count use run_id instead of execution_date fix tests and deprecate mark_tasks fixup! fix tests and deprecate mark_tasks fixup! fixup! fix tests and deprecate mark_tasks fixup! fixup! fixup! fix tests and deprecate mark_tasks fixup! fixup! fixup! fix tests and deprecate mark_tasks fixup! fixup! fixup! fixup! fix tests and deprecate mark_tasks fixup! Ensure task_instance exists before running update on its state(REST API) Use execution_date for run/clear fixup! Use execution_date for run/clear constrain trino Apply suggestions from code review Co-authored-by: Ash Berlin-Taylor <[email protected]> add execution_date remove lib fix tests fixup! fix tests apply reviews from code review Fix typing Fix tests Apply suggestions from code review Co-authored-by: Tzu-ping Chung <[email protected]> fixup! Apply suggestions from code review add find method to task instance name execution_date explicitly fix mypy accept only run_id accept only run_id fixup! accept only run_id Update airflow/models/taskinstance.py Co-authored-by: Tzu-ping Chung <[email protected]> update docstring
Co-authored-by: Ash Berlin-Taylor <[email protected]>
ephraimbuddy
force-pushed
the
add-run-id
branch
from
4f9669a to
e5e60db
Compare
ephraimbuddy
added a commit
to astronomer/airflow
that referenced
this pull request
Jan 14, 2022
When I merged apache#18724 the jobs ran successfully but it's now failing in main. This PR fixes it
Merged
ephraimbuddy
added a commit
that referenced
this pull request
Jan 14, 2022
When I merged #18724 the jobs ran successfully but it's now failing in main. This PR fixes it Co-authored-by: Kaxil Naik <[email protected]> Co-authored-by: Ash Berlin-Taylor <[email protected]>
ephraimbuddy
added
the
type:misc/internal
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We can now use run_id as well as execution_date to update states
of task instances
Depends on #18642
Closes: #18672
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.