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

openlineage: Set slots to True for facets used in dag level #40972

Merged

Conversation

JDarDagran
Copy link
Contributor

Implements approach proposed in #40971.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

state change level OpenLineage listener hooks.

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
@kacpermuda
Copy link
Contributor

Quick question, if the default behaviour changed and all Facets in v2 have the slots=True why not change all the Airflow facets to mimic that and not only the ones that go to DAG events and undergo pickling. Is that impacting the performance a lot? Is there any other point here that we are worried about? For the sake of simplicity and to avoid mistakes i think it would be good to keep it consistent. If not, let's at least leave a comment / docstring somewhere in the code about why sometimes it's False and sometimes it's True.

@JDarDagran
Copy link
Contributor Author

Quick question, if the default behaviour changed and all Facets in v2 have the slots=True why not change all the Airflow facets to mimic that and not only the ones that go to DAG events and undergo pickling. Is that impacting the performance a lot? Is there any other point here that we are worried about? For the sake of simplicity and to avoid mistakes i think it would be good to keep it consistent. If not, let's at least leave a comment / docstring somewhere in the code about why sometimes it's False and sometimes it's True.

I agree, for the sake of simplicity it'd be better to have all facets set slots to True.

@mobuchowski mobuchowski merged commit eca0555 into apache:main Jul 24, 2024
52 checks passed
sunank200 pushed a commit to astronomer/airflow that referenced this pull request Jul 24, 2024
state change level OpenLineage listener hooks.

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
molcay pushed a commit to VladaZakharova/airflow that referenced this pull request Aug 19, 2024
state change level OpenLineage listener hooks.

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants