You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/10/14 04:53:41 UTC
[GitHub] [airflow] gulshngill opened a new pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
gulshngill opened a new pull request #18968:
URL: https://github.com/apache/airflow/pull/18968
A small suggestion to change `DagRun.find` in `trigger_dag` to use `execution_date` as a parameter rather than `run_id`.
I feel it would make more sense to use this rather than `run_id` as a parameter since using `run_id` will miss out checking for a scheduled run that ran at the same `execution_date` and throw the error below when it tries to create a new run at the same `execution_date`:
```
sqlalchemy.exc.IntegrityError: (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "dag_run_dag_id_execution_date_key"
```
There is a constraint in `dag_run` called `dag_run_dag_id_execution_date_key` which can be found [here](https://github.com/apache/airflow/blob/c4f5233cd10ae03ee69fba861c8a6fa64e1f8a71/airflow/models/dagrun.py#L103).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #18968: Use ``execution_date`` to check for existing ``DagRun`` for ``TriggerDagRunOperator``
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-959839443
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r739121363
##########
File path: airflow/models/dagrun.py
##########
@@ -270,7 +270,7 @@ def next_dagruns_to_examine(
def find(
dag_id: Optional[Union[str, List[str]]] = None,
run_id: Optional[str] = None,
- execution_date: Optional[datetime] = None,
+ execution_date: Optional[Union[datetime, List[datetime]]] = None,
Review comment:
Sure, why not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #18968: Use ``execution_date`` to check for existing ``DagRun`` for ``TriggerDagRunOperator``
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #18968:
URL: https://github.com/apache/airflow/pull/18968
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gulshngill commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
gulshngill commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r739079658
##########
File path: airflow/models/dagrun.py
##########
@@ -270,7 +270,7 @@ def next_dagruns_to_examine(
def find(
Review comment:
@uranusjr , I'm thinking while we're updating this file, should we also convert this function into a `classmethod`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-945137772
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r733321814
##########
File path: airflow/models/dagrun.py
##########
@@ -334,6 +334,34 @@ def find(
return qry.order_by(DR.execution_date).all()
+ @staticmethod
+ @provide_session
+ def find_duplicate(
+ dag_id: str,
+ run_id: str,
+ execution_date: datetime,
+ session: Session = None,
+ ) -> Optional['DagRun']:
+ """
+ Returns a dag run if there is an existing run for a dag at a specific run_id and execution_date.
Review comment:
```suggestion
Return an existing run for the DAG with a specific run_id or execution_date.
*None* is returned if no such DAG run is found.
```
I wonder if this function should be on `DAG` instead, since this needs a DAG ID.
If we are going to put this on `DagRun`, this should be a classmethod (so you can do `session.query(cls)` below).
##########
File path: airflow/models/dagrun.py
##########
@@ -334,6 +334,34 @@ def find(
return qry.order_by(DR.execution_date).all()
+ @staticmethod
+ @provide_session
+ def find_duplicate(
+ dag_id: str,
+ run_id: str,
+ execution_date: datetime,
+ session: Session = None,
+ ) -> Optional['DagRun']:
+ """
+ Returns a dag run if there is an existing run for a dag at a specific run_id and execution_date.
+
+ :param dag_id: the dag_id to find duplicates for
+ :type dag_id: str
+ :param run_id: defines the run id for this dag run
+ :type run_id: str
+ :param execution_date: the execution date
+ :type execution_date: datetime.datetime
+ :param session: database session
+ :type session: sqlalchemy.orm.session.Session
+ """
+ DR = DagRun
+
+ qry = session.query(DR)
+
+ qry = qry.filter(DR.dag_id == dag_id, or_(DR.run_id == run_id, DR.execution_date == execution_date))
+
+ return qry.first()
Review comment:
```suggestion
return (
session.query(cls)
.filter(
cls.dag_id == dag_id,
or_(cls.run_id == run_id, cls.execution_date == execution_date),
)
.one_or_none()
)
```
This should always return at most one row since we have a unique constraint on the database, but if it somehow returns multiple ones, we want to be loud about it to raise awareness to the inconsistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gulshngill commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
gulshngill commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r735230939
##########
File path: airflow/models/dagrun.py
##########
@@ -334,6 +334,34 @@ def find(
return qry.order_by(DR.execution_date).all()
+ @staticmethod
+ @provide_session
+ def find_duplicate(
+ dag_id: str,
+ run_id: str,
+ execution_date: datetime,
+ session: Session = None,
+ ) -> Optional['DagRun']:
+ """
+ Returns a dag run if there is an existing run for a dag at a specific run_id and execution_date.
Review comment:
I think it's fine in `DagRun` since the function itself is related to finding DAG runs and has some similarities with `find` function
Let me know your thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-945139090
As a side note, we are moving toward decoupling DAG runs against `execution_date` and removing the unique constraint eventually, and when that happens this code will become (semantically) incorrect. But until that happens, this is the correct thing to do, and wouldn't break anything even after the constraint's removal since it only adds a superfulous restriction on when DAG runs can be created, which is more appropriate than the functionality throwing exceptions right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gulshngill commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
gulshngill commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r732945444
##########
File path: airflow/models/dagrun.py
##########
@@ -270,7 +270,7 @@ def next_dagruns_to_examine(
def find(
dag_id: Optional[Union[str, List[str]]] = None,
run_id: Optional[str] = None,
- execution_date: Optional[datetime] = None,
+ execution_date: Optional[Union[datetime, List[datetime]]] = None,
Review comment:
Not related to this PR but noticed that this did not match the type in the docstring
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r741490816
##########
File path: tests/models/test_dagrun.py
##########
@@ -142,6 +142,29 @@ def test_dagrun_find(self):
assert 0 == len(models.DagRun.find(dag_id=dag_id2, external_trigger=True))
assert 1 == len(models.DagRun.find(dag_id=dag_id2, external_trigger=False))
+ def test_dagrun_find_duplicate(self):
+ session = settings.Session()
+ now = timezone.utcnow()
+
+ dag_id1 = "test_dagrun_find_duplicate"
Review comment:
```suggestion
dag_id = "test_dagrun_find_duplicate"
```
nit: just a better variable name
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #18968: Use ``execution_date`` to check for existing ``DagRun`` for ``TriggerDagRunOperator``
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #18968:
URL: https://github.com/apache/airflow/pull/18968
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #18968: Use ``execution_date`` to check for existing ``DagRun`` for ``TriggerDagRunOperator``
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-959839443
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gulshngill commented on pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
gulshngill commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-947835552
Hi @uranusjr , thanks for taking the time to review this PR and for the suggestion.
I've made the changes as suggested and updated the unit tests as well, I'll continue with doing the full matrix of tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #18968: Use ``execution_date`` to check for existing ``DagRun`` for ``TriggerDagRunOperator``
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-959839443
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #18968: Use ``execution_date`` to check for existing ``DagRun`` for ``TriggerDagRunOperator``
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #18968:
URL: https://github.com/apache/airflow/pull/18968
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-942945651
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gulshngill commented on a change in pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
gulshngill commented on a change in pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#discussion_r732945444
##########
File path: airflow/models/dagrun.py
##########
@@ -270,7 +270,7 @@ def next_dagruns_to_examine(
def find(
dag_id: Optional[Union[str, List[str]]] = None,
run_id: Optional[str] = None,
- execution_date: Optional[datetime] = None,
+ execution_date: Optional[Union[datetime, List[datetime]]] = None,
Review comment:
Noticed that this did not match the type in the docstring
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #18968: Suggestion to change DagRun.find in trigger_dag to use execution_date instead of run_id
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18968:
URL: https://github.com/apache/airflow/pull/18968#issuecomment-945336188
Actually, DagRun has a unique constraint on _both_ `execution_date` and `run_id`. So while this fixes the duplicated `execution_date` issue, it'd introduce a new issue of duplicated `run_id` (which is possible with manual runs, where the user can customise `run_id`). So this should do a SQL `WHERE run_id = ? OR execution_date = ?` query instead. `DagRun.find()` does not support this, but I think the lookup logic is common enough to warrant a new method, say `DagRun.find_duplicate(dag_id, run_id, execution_date)` (where all three arguments are required). There is actually already at least one occurence, maybe more (I didn't search very hard).
https://github.com/apache/airflow/blob/86a2a19ad2bdc87a9ad14bb7fde9313b2d7489bb/airflow/api_connexion/endpoints/dag_run_endpoint.py#L256-L263
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org