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

Generalize caching of connection in DbApiHook to improve performance #40751

Merged
merged 91 commits into from
Sep 4, 2024

Conversation

dabla
Copy link
Contributor

@dabla dabla commented Jul 12, 2024

Added a cached connection property in DbApiHook to improve performance (it already existed for MySQLHook and MSSQLHook), also move the duplicated connection_extra_lower property from JdbcHook and OdbcHook to DbApiHook. This enchancement will fix the unnecessary connection lookups for Jdbc and Odbc driver properties.


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

…DbApiHook and added cached connection_extra property
@dabla
Copy link
Contributor Author

dabla commented Jul 13, 2024

I'm just wondering if I have to bump to version of the common sql provider? As there will be a change there, it should become like 1.15.0 as @potiuk suggested in the other PR 40665. Has this to be done manually (I don't think so but could), I've also read the docs here but didn't find the answer to that.

@dabla
Copy link
Contributor Author

dabla commented Jul 15, 2024

One test failing due to:

AttributeError: type object 'SkipDBTestsSession' has no attribute 'get_bind'

@dabla
Copy link
Contributor Author

dabla commented Jul 15, 2024

One test failing due to:

AttributeError: type object 'SkipDBTestsSession' has no attribute 'get_bind'

Seems my other PR are also having the same issue

@dabla dabla changed the title Add cached property for connection_extra in DbApiHook to improve performance Generalize caching of connection in DbApiHook to improve performance Jul 15, 2024
@eladkal
Copy link
Contributor

eladkal commented Aug 25, 2024

Looks like we need now to set min version for apache-airflow-providers-common-sql as 1.17.0
otherwise LGTM

@eladkal
Copy link
Contributor

eladkal commented Sep 1, 2024

@dabla can you make the needed change so we can merge this for next release?

@dabla
Copy link
Contributor Author

dabla commented Sep 1, 2024

@dabla can you make the needed change so we can merge this for next release?

Will do that tomorrow first thing in the morning

@potiuk
Copy link
Member

potiuk commented Sep 3, 2024

Hey @dabla -> you need to also upgrade common.sql version in their provider.yaml - otherwise PROD image build is not able to find common.sql >= 1.17.0.dev0

The build is smart enough (or it should be at least) to figure out that in this case the common.sql package should be buillt and used for PROD image build - but you need to bump the version in it's provider.yaml.

@dabla
Copy link
Contributor Author

dabla commented Sep 3, 2024

Hey @dabla -> you need to also upgrade common.sql version in their provider.yaml - otherwise PROD image build is not able to find common.sql >= 1.17.0.dev0

The build is smart enough (or it should be at least) to figure out that in this case the common.sql package should be buillt and used for PROD image build - but you need to bump the version in it's provider.yaml.

Aha, that's why the image build is failing, thanks for letting me know @potiuk, will do that

@dabla
Copy link
Contributor Author

dabla commented Sep 3, 2024

Hey @dabla -> you need to also upgrade common.sql version in their provider.yaml - otherwise PROD image build is not able to find common.sql >= 1.17.0.dev0

The build is smart enough (or it should be at least) to figure out that in this case the common.sql package should be buillt and used for PROD image build - but you need to bump the version in it's provider.yaml.

@potiuk What about the release notes and stuff for common sql, as when I bump in provider.yaml, do I also need to bump that version init.py file and release notes? Or is there a (breeze) command to bump the version of a provider? Didn't directly find it in docs but that's probably my bad.

@potiuk
Copy link
Member

potiuk commented Sep 3, 2024

Just add version in the provider.yaml. all the rest should happen by pre-commits

You might want to add a line at top of changelog explaining better what was added in common.sql (but this is usually only done for breaking changes - generally commit messages are added to the changelog when release manager prepares the release.

@dabla
Copy link
Contributor Author

dabla commented Sep 3, 2024

Just add version in the provider.yaml. all the rest should happen by pre-commits

You might want to add a line at top of changelog explaining better what was added in common.sql (but this is usually only done for breaking changes - generally commit messages are added to the changelog when release manager prepares the release.

Nice, ok thank you @potiuk , wanted to make sure before bumping the version, will add a comment line in top of file explaining what was changed, always better to document anyway and good for the changelog history.

@potiuk
Copy link
Member

potiuk commented Sep 3, 2024

Nice, ok thank you @potiuk , wanted to make sure before bumping the version, will add a comment line in top of file explaining what was changed, always better to document anyway and good for the changelog history.

Not sure if it will work with out release management preparation automation if you added the Features header (normally it is added automatically when provider bulk documentation is being prepared, but we might see if it works. CC: @eladkal @amoghrajesh

@dabla
Copy link
Contributor Author

dabla commented Sep 3, 2024

Nice, ok thank you @potiuk , wanted to make sure before bumping the version, will add a comment line in top of file explaining what was changed, always better to document anyway and good for the changelog history.

Not sure if it will work with out release management preparation automation if you added the Features header (normally it is added automatically when provider bulk documentation is being prepared, but we might see if it works. CC: @eladkal @amoghrajesh

If it causes issues, I can revert it as I added this in a dedicated commit.

@potiuk
Copy link
Member

potiuk commented Sep 3, 2024

If it causes issues, I can revert it as I added this in a dedicated commit.

We will see and RM can modify it on-line. How it works is that it simply updates the changelog locally and RM commits it, so if it causes issues, it can be manually modified before running the script and classifying the issues. Or we can implement a fix to allow that in the future.

@potiuk potiuk merged commit 2e813eb into apache:main Sep 4, 2024
111 of 112 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.

5 participants