You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "michaelmicheal (via GitHub)" <gi...@apache.org> on 2023/02/09 16:22:20 UTC
[GitHub] [airflow] michaelmicheal opened a new pull request, #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
michaelmicheal opened a new pull request, #29441:
URL: https://github.com/apache/airflow/pull/29441
Closes: #26892
I'm not sure what the intention of this filter was:
`DatasetEvent.timestamp > DatasetDagRunQueue.created_at`
When a dataset is updated,
1. A `DatasetEvent` is created (and saved)
2. And then, a `DatasetDagRunQueue` row is added for each dependant DAG.
I don't think we need this filter, and have confirmed that it prevents the last update from being shown.
By removing this filter, it resolves #26892
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] blag commented on a diff in pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "blag (via GitHub)" <gi...@apache.org>.
blag commented on code in PR #29441:
URL: https://github.com/apache/airflow/pull/29441#discussion_r1102128972
##########
airflow/www/views.py:
##########
@@ -3715,7 +3715,6 @@ def next_run_datasets(self, dag_id):
DatasetEvent,
and_(
DatasetEvent.dataset_id == DatasetModel.id,
- DatasetEvent.timestamp > DatasetDagRunQueue.created_at,
Review Comment:
😬 It's been a long minute since I wrote this, but...
I believe when I wrote this the intent with the `lastUpdate` field was to only show last updates _since the last time the dag was queued/run_. But yeah, the `lastUpdate` label isn't descriptive enough for that.
Option 1: Personally, I would consider changing the name of what the `lastUpdate` field is rendered to, something like "Last update since last run" or something more wordy.
Option 2: But if you don't want to do that, and you want to display the last update for every dataset regardless of whether has already been "consumed" by a DagRun (eg: in either the DatasetDagRunQueue or actually scheduled into a DagRun), then yeah it makes sense to remove this filter. However, I would also remove the `and_` around it since then there would only be one filter condition in that join:
```python
.join(
DatasetEvent,
DatasetEvent.dataset_id == DatasetModel.id,
isouter=True,
)
```
If you go for option 2, I think you should be able to compare the existence and creation time of the DDRQ with the DatasetEvent timestamp to figure out whether or not the last update time has already triggered a DDRQ/DagRun or if it has partially satisfied the conditions of a future DagRun.
Hope this makes sense.
--
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] michaelmicheal commented on pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on PR #29441:
URL: https://github.com/apache/airflow/pull/29441#issuecomment-1433180686
@ephraimbuddy @ashb Is this good to merge?
--
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] potiuk merged pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29441:
URL: https://github.com/apache/airflow/pull/29441
--
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] michaelmicheal commented on a diff in pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on code in PR #29441:
URL: https://github.com/apache/airflow/pull/29441#discussion_r1107288797
##########
airflow/www/views.py:
##########
@@ -3715,7 +3715,6 @@ def next_run_datasets(self, dag_id):
DatasetEvent,
and_(
DatasetEvent.dataset_id == DatasetModel.id,
- DatasetEvent.timestamp > DatasetDagRunQueue.created_at,
Review Comment:
@blag Does that make sense to you?
--
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] michaelmicheal commented on pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on PR #29441:
URL: https://github.com/apache/airflow/pull/29441#issuecomment-1436166837
@potiuk is this good to merge?
--
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] bbovenzi commented on a diff in pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #29441:
URL: https://github.com/apache/airflow/pull/29441#discussion_r1101776764
##########
airflow/www/views.py:
##########
@@ -3715,7 +3715,6 @@ def next_run_datasets(self, dag_id):
DatasetEvent,
and_(
DatasetEvent.dataset_id == DatasetModel.id,
- DatasetEvent.timestamp > DatasetDagRunQueue.created_at,
Review Comment:
@blag Does this look right to you? re: https://github.com/apache/airflow/pull/26356
--
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] blag commented on a diff in pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "blag (via GitHub)" <gi...@apache.org>.
blag commented on code in PR #29441:
URL: https://github.com/apache/airflow/pull/29441#discussion_r1107726702
##########
airflow/www/views.py:
##########
@@ -3715,7 +3715,6 @@ def next_run_datasets(self, dag_id):
DatasetEvent,
and_(
DatasetEvent.dataset_id == DatasetModel.id,
- DatasetEvent.timestamp > DatasetDagRunQueue.created_at,
Review Comment:
I just dug through the code in more detail, and yes, your understanding seems to be correct. 😄
--
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] blag commented on a diff in pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "blag (via GitHub)" <gi...@apache.org>.
blag commented on code in PR #29441:
URL: https://github.com/apache/airflow/pull/29441#discussion_r1102128972
##########
airflow/www/views.py:
##########
@@ -3715,7 +3715,6 @@ def next_run_datasets(self, dag_id):
DatasetEvent,
and_(
DatasetEvent.dataset_id == DatasetModel.id,
- DatasetEvent.timestamp > DatasetDagRunQueue.created_at,
Review Comment:
😬 It's been a long minute since I wrote this, but...
I believe when I wrote this the intent with the `lastUpdate` field was to only show last updates _since the last time the dag was queued/run_. But yeah, the `lastUpdate` label isn't descriptive enough for that.
Option 1: Personally, I would consider changing the name of what the `lastUpdate` field is rendered to, something like "Last update since last run" or something more wordy.
Option 2: But if you don't want to do that, and you want to display the last update for every dataset regardless of whether has already been "consumed" by a DagRun (eg: in either the DatasetDagRunQueue or actually scheduled into a DagRun), then yeah it makes sense to remove this filter. However, I would also remove the `and_` around it since then there would only be one filter condition in that join:
```python
.join(
DatasetEvent,
DatasetEvent.dataset_id == DatasetModel.id,
isouter=True,
)
```
If you go for option 2, I think you should be able to compare the existence and creation time of the DDRQ with the DatasetEvent timestamp to figure out whether or not the last update time has already triggered a DagRun or if it has partially satisfied the conditions of a future DagRun.
Hope this makes sense.
--
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] michaelmicheal commented on a diff in pull request #29441: datasets, next_run_datasets, remove unnecessary timestamp filter
Posted by "michaelmicheal (via GitHub)" <gi...@apache.org>.
michaelmicheal commented on code in PR #29441:
URL: https://github.com/apache/airflow/pull/29441#discussion_r1102819882
##########
airflow/www/views.py:
##########
@@ -3715,7 +3715,6 @@ def next_run_datasets(self, dag_id):
DatasetEvent,
and_(
DatasetEvent.dataset_id == DatasetModel.id,
- DatasetEvent.timestamp > DatasetDagRunQueue.created_at,
Review Comment:
> However, I would also remove the and_ around it since then there would only be one filter condition in that join:
Yes, you're right the `and` becomes unnecessary.
I think there might be some confusion around DDRQ. My understanding is that when a `DatasetEvent` is created, a DDRQ record is created per consuming DAG. Then, once a DAG has an associated DDRQ record for each `Dataset` that it depends on, a dag_run is created and then all DDRQ records associated with that DAG are deleted.
> If you go for option 2, I think you should be able to compare the existence and creation time of the DDRQ with the DatasetEvent timestamp to figure out whether or not the last update time has already triggered a DDRQ/DagRun or if it has partially satisfied the conditions of a future DagRun.
As I understand it, if there are DDRQ records for a DAG, we can assume that there hasn't been a DagRun triggered since the last DatasetEvent (because we delete DDRQ records on the creation of a DagRun).
--
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