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/11/04 19:13:15 UTC
[GitHub] [airflow] o-nikolas opened a new pull request #19410: Remove inaccurate execution date from triggered dag extra link
o-nikolas opened a new pull request #19410:
URL: https://github.com/apache/airflow/pull/19410
Mitigate #19264 such that the extra link from the triggered dag at least loads the current date and
displays all current dag runs runs. See that issue for further discussion on future DB changes.
<!--
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 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] ashb commented on a change in pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#discussion_r758268190
##########
File path: airflow/operators/trigger_dagrun.py
##########
@@ -156,6 +166,11 @@ def execute(self, context: Dict):
else:
raise e
+ # Store the execution date from the dag run (either created or found above) to
+ # be used when creating the extra link on the webserver.
+ ti = context['task_instance']
+ ti.xcom_push(key=XCOM_EXECUTION_DATE_ISO, value=dag_run.execution_date.isoformat())
Review comment:
```suggestion
ti.xcom_push(key=XCOM_EXECUTION_DATE_ISO, value=dag_run.execution_date.isoformat())
ti.xcom_push(key="run_id", value=dag_run.run_id)
```
Too plese
--
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] ashb commented on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-965042741
This fix doesn't look right to me -- by not including the execution date it will link to the latest run instead.
--
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] ljades commented on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
ljades commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-965361221
> This fix doesn't look right to me -- by not including the execution date it will link to the latest run instead.
@ashb
The issue working to be mitigated here is that the execution date parameter wasn't being used properly at all anyway and was linking to a poorly filtered view of the DAG, so mitigating it at least allows the user to view the latest and filter easily to the target.
--
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] o-nikolas edited a comment on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas edited a comment on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-965562760
> > This fix doesn't look right to me -- by not including the execution date it will link to the latest run instead.
>
> @ashb The issue working to be mitigated here is that the execution date parameter wasn't being used properly at all anyway and was linking to a poorly filtered view of the DAG, so mitigating it at least allows the user to view the latest and filter easily to the target.
Hey @ashb,
@ljades summarized it correctly. I know the original issue is quite long, but if you read that, it that will provide a lot of context so that this makes sense. Specifically the latest comment [here](https://github.com/apache/airflow/issues/19264#issuecomment-955070130). This PR basically does 1) from that comment.
Basically a "real fix" is quite tricky and I wasn't getting much feedback in the ticket, so as a stop gap this PR will at least remove the filter so that you see all runs from current run back which seemed like a decent default for the time being.
--
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] o-nikolas commented on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-962191295
@ljades @uranusjr Quick PR to remove the erroneous execution_date from the triggered url.
@uranusjr: Two of the tests are failing and I have no idea why. I can't reproduce it locally (those tests pass when I run them) and they aren't producing an exception or any logging, it just times out at 68% completion. Any ideas on how I can move forward here?
--
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] o-nikolas commented on a change in pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#discussion_r759580709
##########
File path: airflow/operators/trigger_dagrun.py
##########
@@ -156,6 +166,11 @@ def execute(self, context: Dict):
else:
raise e
+ # Store the execution date from the dag run (either created or found above) to
+ # be used when creating the extra link on the webserver.
+ ti = context['task_instance']
+ ti.xcom_push(key=XCOM_EXECUTION_DATE_ISO, value=dag_run.execution_date.isoformat())
Review comment:
Thanks for the context, it's helpful to add along with requested code changes! I'll push an update to the PR shortly.
--
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] o-nikolas commented on a change in pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#discussion_r758635517
##########
File path: airflow/operators/trigger_dagrun.py
##########
@@ -156,6 +166,11 @@ def execute(self, context: Dict):
else:
raise e
+ # Store the execution date from the dag run (either created or found above) to
+ # be used when creating the extra link on the webserver.
+ ti = context['task_instance']
+ ti.xcom_push(key=XCOM_EXECUTION_DATE_ISO, value=dag_run.execution_date.isoformat())
Review comment:
Hey @ashb, Sure I can make this change, but I'm curious, why do you want the run id pushed as well? Is there some other feature that depends on it? It isn't required for this feature as far as I can tell.
--
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 #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-964583517
> it just times out at 68% completion. Any ideas on how I can move forward here?
Don't worry about those, the CI runner timed out.
--
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] ashb merged pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #19410:
URL: https://github.com/apache/airflow/pull/19410
--
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] o-nikolas edited a comment on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas edited a comment on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-965562760
> > This fix doesn't look right to me -- by not including the execution date it will link to the latest run instead.
>
> @ashb The issue working to be mitigated here is that the execution date parameter wasn't being used properly at all anyway and was linking to a poorly filtered view of the DAG, so mitigating it at least allows the user to view the latest and filter easily to the target.
Hey @ashb,
@ljades summarized it correctly. I know the original issue is quite long, but if you read that, it that will provide a lot of context so that this makes sense.
Basically a "real fix" is quite tricky and I wasn't getting much feedback in the ticket, so as a stop gap this PR will at least remove the filter so that you see all runs from current run back which seemed like a decent default for the time being.
--
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] ashb commented on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-966380955
From that issue:
> So the webserver does not have access to the results of those computations when it's building the link
Store the triggered execution date in XCom and pull it in the operator link, like
https://github.com/apache/airflow/blob/7622f5e08261afe5ab50a08a6ca0804af8c7c7fe/airflow/providers/google/cloud/operators/bigquery.py#L737
and use it like
https://github.com/apache/airflow/blob/7622f5e08261afe5ab50a08a6ca0804af8c7c7fe/airflow/providers/google/cloud/operators/bigquery.py#L63-L69
--
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] o-nikolas commented on pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-980453481
Hey @ashb, do you have a minute to take another look at this PR? Appreciate it!
--
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] ashb commented on a change in pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#discussion_r759254785
##########
File path: airflow/operators/trigger_dagrun.py
##########
@@ -156,6 +166,11 @@ def execute(self, context: Dict):
else:
raise e
+ # Store the execution date from the dag run (either created or found above) to
+ # be used when creating the extra link on the webserver.
+ ti = context['task_instance']
+ ti.xcom_push(key=XCOM_EXECUTION_DATE_ISO, value=dag_run.execution_date.isoformat())
Review comment:
Not _right_ now, but I am slowly working towards making Airflow be able to have multiple runs for the _same_ date, so by adding this now it future proofs us and means we can change the URL without having to worry about backwards compat of "did we push run_id or 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] o-nikolas commented on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-969268405
> Store date/runid (both please) in XCom and use to build the link.
Thanks for the feedback Ash! I'll play around with this and get back to this PR soon :+1:
--
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] o-nikolas commented on pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-987453722
Hey @ashb your requested changes should be in. Do you have time to have a quick review of this one?
--
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 closed pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #19410:
URL: https://github.com/apache/airflow/pull/19410
--
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] o-nikolas commented on pull request #19410: Remove inaccurate execution date from triggered dag extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#issuecomment-965562760
> > This fix doesn't look right to me -- by not including the execution date it will link to the latest run instead.
>
> @ashb The issue working to be mitigated here is that the execution date parameter wasn't being used properly at all anyway and was linking to a poorly filtered view of the DAG, so mitigating it at least allows the user to view the latest and filter easily to the target.
Hey @ashb,
@ljades summarized it correctly. I know the original issue is quite long, but if you read that, it that will provide a lot of context so that 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] o-nikolas commented on a change in pull request #19410: Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #19410:
URL: https://github.com/apache/airflow/pull/19410#discussion_r758635517
##########
File path: airflow/operators/trigger_dagrun.py
##########
@@ -156,6 +166,11 @@ def execute(self, context: Dict):
else:
raise e
+ # Store the execution date from the dag run (either created or found above) to
+ # be used when creating the extra link on the webserver.
+ ti = context['task_instance']
+ ti.xcom_push(key=XCOM_EXECUTION_DATE_ISO, value=dag_run.execution_date.isoformat())
Review comment:
Hey @ashb, Sure I can make this change, but I'm curious, why do you want the run id pushed as well? Is there some other feature that depends on it? It isn't required for this feature as far as I can tell.
I'm probably missing something obvious XD
--
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