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

Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver #38707

Merged
merged 37 commits into from
Apr 12, 2024

Conversation

dabla
Copy link
Contributor

@dabla dabla commented Apr 3, 2024

We experienced unexpected exceptions being raised by the JdbcHook as it is invoking the setAutoCommit and getAutoCommit methods on the JDBC driver, but in our case as we are using the SAS JDBC driver, those methods throw an UnsupportedOperationException in Java. Even though those methods make part of the JDBC spec, it isn't always implemented by the driver hence why the exception. So to avoid those exceptions to be propagated, it's better to suppress them.


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

…it is being invoked as some JDBC drivers don't support this operation
@dabla dabla requested a review from potiuk April 9, 2024 15:06
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

There is a problem with back-compatibility. Our providers work for Airflow >= 2.6.0 - see the errrors raised in tests - it cannot be imported from Airlfow until we keep 2.10.0 support - you will need to duplicate it in the provider and fall-back to it and mark it as "remove after min-airflow version is set to 2.10.

Another option (probably better) is to add it to common.sql and use from there (and add common.sql >= NEXT MINOR VERSION in provider.yaml.

@dabla
Copy link
Contributor Author

dabla commented Apr 10, 2024

There is a problem with back-compatibility. Our providers work for Airflow >= 2.6.0 - see the errrors raised in tests - it cannot be imported from Airlfow until we keep 2.10.0 support - you will need to duplicate it in the provider and fall-back to it and mark it as "remove after min-airflow version is set to 2.10.

Another option (probably better) is to add it to common.sql and use from there (and add common.sql >= NEXT MINOR VERSION in provider.yaml.

Yes I saw that and I have same issue I think with MSGraphOperator but I don't fully understand how I can fix this :(

@potiuk
Copy link
Member

potiuk commented Apr 10, 2024

Yes I saw that and I have same issue I think with MSGraphOperator but I don't fully understand how I can fix this :(

Simply - You can't use code from airfliow in providers until the provider has apache-airflow>=NEXT_MINOR - until then the provider must have a "polyfill" - i.e. catch import error and have the same code that is in the provider that providers same functionality when provider is installed on airflow < NEXT_MINOR. With a note to remove it when min-airflow version is >= NEXT_MINOR (we have a policy for bumping - in two weeks we increase min-airflow-version to 2.7.0, so after that any code that was there fore < 2.7 can be removed from providers.

For SQL providers, a simpler way around it is to add a code to common.sql as a new feature and use apache-airfow-provider-common-sql >= in provider.yaml` - but then it should be visibly marked as added in common.sql x.y.z (via since flag in the docuemntation).

There was a discussion to have common.util provider at some point of time but it stalled a bit. Maybe we should come back to it.

@dabla
Copy link
Contributor Author

dabla commented Apr 11, 2024

Yes I saw that and I have same issue I think with MSGraphOperator but I don't fully understand how I can fix this :(

Simply - You can't use code from airfliow in providers until the provider has apache-airflow>=NEXT_MINOR - until then the provider must have a "polyfill" - i.e. catch import error and have the same code that is in the provider that providers same functionality when provider is installed on airflow < NEXT_MINOR. With a note to remove it when min-airflow version is >= NEXT_MINOR (we have a policy for bumping - in two weeks we increase min-airflow-version to 2.7.0, so after that any code that was there fore < 2.7 can be removed from providers.

For SQL providers, a simpler way around it is to add a code to common.sql as a new feature and use apache-airfow-provider-common-sql >= in provider.yaml` - but then it should be visibly marked as added in common.sql x.y.z (via since flag in the docuemntation).

There was a discussion to have common.util provider at some point of time but it stalled a bit. Maybe we should come back to it.

Ok got it, thank you for the explantion, indeed I saw that discussion of the common.util. At the moment I just can move that suppress_and_warn context manager to the sql provider as it isn't used elsewhere, but I put it in airflow itself which indeed started the dependency issue. We can always move it afterwards if it's needed elsewhere, like for example to a common util.

@dabla dabla requested a review from eladkal as a code owner April 11, 2024 05:59
@dabla
Copy link
Contributor Author

dabla commented Apr 11, 2024

2. common-sql-provider dependencies

Ok thanks @potiuk will do that 👍 Conflict has already been resolved

@dabla
Copy link
Contributor Author

dabla commented Apr 11, 2024

@potiuk You're sure I need to update the dependency in all providers? Because I checked and I see not all providers depend on the same version. I just ask to be sure so I don't break any provider.
image

@potiuk
Copy link
Member

potiuk commented Apr 11, 2024

Yes. But don't do it yet indeed.

We have no policy for that and this is something I just realised we will need to fix - becuase some of the providers might depend on features that were released since their min version. In most installation they already have latest common.sql because it is set like that in constraints in the new

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

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

I would like to avoid use common.sql as kind of common.collection_of_the_unknown_utilites

airflow/providers/common/sql/hooks/sql.py Outdated Show resolved Hide resolved
….0 to 2.7.0 for microsoft-azure provider in provider_dependencies.json
@potiuk
Copy link
Member

potiuk commented Apr 11, 2024

Also pre-commit needs to be run @dabla, see the error here: https://github.com/apache/airflow/actions/runs/8645779337/job/23703693429?pr=38707

…from 2.6.0 to 2.7.0 for microsoft-azure provider in provider_dependencies.json"

This reverts commit 63982cb.
@dabla
Copy link
Contributor Author

dabla commented Apr 11, 2024

Also pre-commit needs to be run @dabla, see the error here: https://github.com/apache/airflow/actions/runs/8645779337/job/23703693429?pr=38707

Yes I thought I reverted that one as I've adapted it in the wrong branch, it's reverted now

@dabla dabla requested a review from Taragolis April 11, 2024 19:02
@potiuk potiuk merged commit 41869d3 into apache:main Apr 12, 2024
41 checks passed
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.

4 participants