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

Moved common SQL handler methods of common-sql-provider into dedicated module #43747

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

dabla
Copy link
Contributor

@dabla dabla commented Nov 6, 2024

This PR is linked to the original PR Introduce notion of dialects in DbApiHook, but only contains a small refactoring in which the sql handlers of the common-sql -provider are moved from the sql module to a dedicated handler module, as this will be needed later on to avoid circular import issues. This is to make the review easier as asked by @potiuk. This PR only impacts the common-sql provider, also a dedicated unit test has been added for the handlers.


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

@dabla
Copy link
Contributor Author

dabla commented Nov 7, 2024

Some things are failing but dunno why

@potiuk
Copy link
Member

potiuk commented Nov 7, 2024

It seems like installing node to build packages is very unstable recently (connection reset by peer etc.) - we will get it cached soon WIP is here: #43329 cc: @bugraoz93 that should improve stability.

@potiuk
Copy link
Member

potiuk commented Nov 7, 2024

For now rebasing, commit--amend or close/reopening the PR should retry it.

@dabla
Copy link
Contributor Author

dabla commented Nov 7, 2024

It seems like installing node to build packages is very unstable recently (connection reset by peer etc.) - we will get it cached soon WIP is here: #43329 cc: @bugraoz93 that should improve stability.

Isn't there a way to be able to re-run a failing step in the ci cd checks of github?

@dabla
Copy link
Contributor Author

dabla commented Nov 7, 2024

For now rebasing, commit--amend or close/reopening the PR should retry it.

Yeah just did it again, I was a bit worried cause my other PR didn't suffer from those random errors so I though maybe something was wrong in this one.

@bugraoz93
Copy link
Collaborator

For now rebasing, commit--amend or close/reopening the PR should retry it.

Yeah just did it again, I was a bit worried cause my other PR didn't suffer from those random errors so I though maybe something was wrong in this one.

This is purely too many trials on node servers trying to download the dependencies and appear in random CI runs

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.

5 participants