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

Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link #19410

Merged
merged 1 commit into from Dec 9, 2021

Conversation

o-nikolas
Copy link
Contributor

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.


^ 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.

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Nov 4, 2021
@o-nikolas o-nikolas force-pushed the onikolas/extra_links_trigger_dag branch from dc24ad5 to ec88ebc Compare November 5, 2021 18:29
@o-nikolas
Copy link
Contributor Author

@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?

@o-nikolas o-nikolas force-pushed the onikolas/extra_links_trigger_dag branch 2 times, most recently from 122447a to 6fb4bd6 Compare November 9, 2021 19:30
@potiuk potiuk added the debug ci resources Set it on PR if you want to debug resource usage for it label Nov 9, 2021
@potiuk potiuk closed this Nov 9, 2021
@potiuk potiuk reopened this Nov 9, 2021
@uranusjr
Copy link
Member

uranusjr commented Nov 9, 2021

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.

@ashb
Copy link
Member

ashb commented Nov 10, 2021

This fix doesn't look right to me -- by not including the execution date it will link to the latest run instead.

@ljades
Copy link

ljades commented Nov 10, 2021

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.

@o-nikolas
Copy link
Contributor Author

o-nikolas commented Nov 10, 2021

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. 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.

@o-nikolas o-nikolas force-pushed the onikolas/extra_links_trigger_dag branch 2 times, most recently from 6ac6981 to 9beb1c7 Compare November 11, 2021 00:11
@ashb
Copy link
Member

ashb commented Nov 11, 2021

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

context['task_instance'].xcom_push(key='job_id', value=job_id)

and use it like

job_id = XCom.get_one(
dag_id=operator.dag.dag_id,
task_id=operator.task_id,
execution_date=dttm,
key='job_id',
)
return BIGQUERY_JOB_DETAILS_LINK_FMT.format(job_id=job_id) if job_id else ''

@ashb ashb self-requested a review November 11, 2021 15:14
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store date/runid (both please) in XCom and use to build the link.

@o-nikolas
Copy link
Contributor Author

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 👍

@o-nikolas o-nikolas force-pushed the onikolas/extra_links_trigger_dag branch from 9beb1c7 to 752575c Compare November 19, 2021 19:16
@uranusjr uranusjr changed the title Remove inaccurate execution date from triggered dag extra link Use XCom to correctly display the execution date from triggered DAG in TriggerDagOperator's extra link Nov 19, 2021
@uranusjr uranusjr requested a review from ashb November 19, 2021 19:41
@o-nikolas
Copy link
Contributor Author

Hey @ashb, do you have a minute to take another look at this PR? Appreciate it!

The extra link provided by the operator was previously using the
execution date of the triggering dag, not the triggered dag. Store the
execution date of the triggered dag in xcom so that it can be read back
later within the webserver when the link is being created.
@o-nikolas o-nikolas force-pushed the onikolas/extra_links_trigger_dag branch from 752575c to 6da11f4 Compare December 1, 2021 00:28
@uranusjr uranusjr requested a review from ashb December 1, 2021 08:52
@o-nikolas
Copy link
Contributor Author

Hey @ashb your requested changes should be in. Do you have time to have a quick review of this one?

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. LGTM!

@ashb ashb merged commit 820e836 into apache:main Dec 9, 2021
@ashb ashb added this to the Airflow 2.3.0 milestone Dec 9, 2021
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Feb 8, 2022
jedcunningham pushed a commit that referenced this pull request Feb 8, 2022
The extra link provided by the operator was previously using the
execution date of the triggering dag, not the triggered dag. Store the
execution date of the triggered dag in xcom so that it can be read back
later within the webserver when the link is being created.

(cherry picked from commit 820e836)
jedcunningham pushed a commit that referenced this pull request Feb 10, 2022
The extra link provided by the operator was previously using the
execution date of the triggering dag, not the triggered dag. Store the
execution date of the triggered dag in xcom so that it can be read back
later within the webserver when the link is being created.

(cherry picked from commit 820e836)
jedcunningham pushed a commit that referenced this pull request Feb 17, 2022
The extra link provided by the operator was previously using the
execution date of the triggering dag, not the triggered dag. Store the
execution date of the triggered dag in xcom so that it can be read back
later within the webserver when the link is being created.

(cherry picked from commit 820e836)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow debug ci resources Set it on PR if you want to debug resource usage for it type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants