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

Add Deferrable switch to SnowflakeSqlApiOperator #31596

Merged
merged 22 commits into from
Jul 4, 2023

Conversation

utkarsharma2
Copy link
Contributor

This PR donates the SnowflakeSqlApiOperator deferrable developed in astronomer-providers repo to Apache airflow.

@boring-cyborg boring-cyborg bot added area:providers kind:documentation provider:snowflake Issues related to Snowflake provider labels May 29, 2023
@utkarsharma2 utkarsharma2 marked this pull request as draft May 29, 2023 13:24
@utkarsharma2 utkarsharma2 marked this pull request as ready for review May 30, 2023 04:30
utkarsharma2 and others added 2 commits June 13, 2023 16:00
Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

lgtm

airflow/providers/snowflake/hooks/snowflake_sql_api.py Outdated Show resolved Hide resolved
Comment on lines +274 to +275
resp = await response.json()
return self._process_response(status_code, resp)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should make the response body be fetched lazily, what could take a while (and might actually be the part worth deferring—not sure)

Copy link
Contributor Author

@utkarsharma2 utkarsharma2 Jun 19, 2023

Choose a reason for hiding this comment

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

@uranusjr I'm not sure I understand you completely, but based on what I got. The function get_sql_api_query_status_async is called indirectly when we are polling for the status of the query within triggered. So deferring it would not be ideal, right?

utkarsharma2 and others added 2 commits June 16, 2023 14:12
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
utkarsharma2 and others added 4 commits June 19, 2023 07:29
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@potiuk
Copy link
Member

potiuk commented Jul 3, 2023

I think you should rebase/solve conflicts on that one jsut to be sure.

@potiuk potiuk merged commit 891c2e4 into apache:main Jul 4, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants