You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dabla (via GitHub)" <gi...@apache.org> on 2024/07/12 14:04:05 UTC
[PR] Add cached property for connection_extra in DbApiHook to improve performance [airflow]
dabla opened a new pull request, #40751:
URL: https://github.com/apache/airflow/pull/40751
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: [40609](https://github.com/apache/airflow/issues/40609)
related: [40609](https://github.com/apache/airflow/issues/40609)
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Add cached property for connection_extra in DbApiHook to improve performance, 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](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2277785064
Don't understand the error as we didn't modify that code:
```
airflow/providers/microsoft/azure/hooks/wasb.py:636: error: Return type
"azure.storage.blob.aio._blob_client_async.BlobClient" of "_get_blob_client"
incompatible with return type "azure.storage.blob._blob_client.BlobClient" in
supertype "WasbHook" [override]
def _get_blob_client(self, container_name: str, blob_name: str) ->...
^
airflow/providers/microsoft/azure/hooks/wasb.py:659: error: Return type
"azure.storage.blob.aio._container_client_async.ContainerClient" of
"_get_container_client" incompatible with return type
"azure.storage.blob._container_client.ContainerClient" in supertype "WasbHook"
[override]
def _get_container_client(self, container_name: str) -> AsyncConta...
^
Found 2 errors in 1 file (checked 3189 source files)
Error 1 returned
You are running mypy with the folders selected. If you want to reproduce it locally, you need to run the following command:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2272739543
@dabla can you rebase and resolve conflicts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2273837070
Also have static check failing on code I didn't change: https://github.com/apache/airflow/actions/runs/10287527178/job/28471099951?pr=40751
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1696381417
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -202,9 +204,28 @@ def placeholder(self):
)
return self._placeholder
+ @property
+ def connection(self) -> Connection:
+ if self._connection is None:
+ self._connection = self.get_connection(self.get_conn_id())
+ return deepcopy(self._connection)
Review Comment:
No idea either, but I just moved that code from the OdbcHook into the DbApiHook, no idea why they did a deepcopy of the connection in the OdbcHook, will be a reason for it...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2323290190
@dabla can you make the needed change so we can merge this for next release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325986475
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1692899100
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -202,9 +204,28 @@ def placeholder(self):
)
return self._placeholder
+ @property
+ def connection(self) -> Connection:
+ if self._connection is None:
+ self._connection = self.get_connection(self.get_conn_id())
+ return deepcopy(self._connection)
Review Comment:
Interesting approach. I think I don't see any big problem except that it will be a Connection ORM object that gets copied and I am not sure what happens in this case and whether there is a problem with some memory leaks and references potentially?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2273602825
> @dabla can you rebase and resolve conflicts?
I've rebased it twice but didn't have any conflicts? The weird thing I'm having the same tests failing like the [PR](https://github.com/apache/airflow/pull/40751) from Ambika which are totally different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1703190727
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -202,9 +204,28 @@ def placeholder(self):
)
return self._placeholder
+ @property
+ def connection(self) -> Connection:
+ if self._connection is None:
+ self._connection = self.get_connection(self.get_conn_id())
+ return deepcopy(self._connection)
Review Comment:
https://github.com/apache/airflow/blame/main/airflow/providers/odbc/hooks/odbc.py
I don't see usage of `deepcopy` in `OdbcHook`
I'd rather keep it for now without `deepcopy` for the reasons that Jarek stated (memory leaks etc.), and only if we understand its necessity - put it back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cached property for connection_extra in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2226892724
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](https://github.com/apache/airflow/pull/40665/). Has this to be done manually (I don't think so but could), I've also read the [docs ](https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md#open-pr-with-suggested-version-releases) here but didn't find the answer to that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #40751:
URL: https://github.com/apache/airflow/pull/40751
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cached property for connection_extra in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2227922545
One [test ](https://github.com/apache/airflow/actions/runs/9935262152/job/27441226855?pr=40751) failing due to:
AttributeError: type object 'SkipDBTestsSession' has no attribute 'get_bind'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2286282577
> I'm a bit puzzled why I get following errors wiht provider compatibility tests as I didn't do any changes in Airflow itself I would assume it should be fine:
>
> ```
> Traceback (most recent call last):
> File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 466, in get_connection_from_secrets
> conn = secrets_backend.get_connection(conn_id=conn_id)
> File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 78, in wrapper
> with create_session() as session:
> File "/usr/local/lib/python3.8/contextlib.py", line 113, in __enter__
> return next(self.gen)
> File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 35, in create_session
> raise RuntimeError("Session must be set before!")
> RuntimeError: Session must be set before!
> Unable to retrieve connection from secrets backend (MetastoreBackend). Checking subsequent secrets backend.
> Traceback (most recent call last):
> File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 466, in get_connection_from_secrets
> conn = secrets_backend.get_connection(conn_id=conn_id)
> File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 78, in wrapper
> with create_session() as session:
> File "/usr/local/lib/python3.8/contextlib.py", line 113, in __enter__
> return next(self.gen)
> File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 35, in create_session
> raise RuntimeError("Session must be set before!")
> RuntimeError: Session must be set before!
> Could not write logs to wasb://container/remote/log/location/1.log
> Traceback (most recent call last):
> File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/log/wasb_task_handler.py", line 210, in wasb_write
> self.hook.load_string(log, self.wasb_container, remote_log_location, overwrite=True)
> File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 360, in load_string
> self.upload(
> File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 417, in upload
> blob_client = self._get_blob_client(container_name, blob_name)
> File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 232, in _get_blob_client
> return self.blob_service_client.get_blob_client(container=container_name, blob=blob_name)
> File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
> val = self.func(instance)
> File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 157, in blob_service_client
> return self.get_conn()
> File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 161, in get_conn
> conn = self.get_connection(self.conn_id)
> File "/usr/local/lib/python3.8/site-packages/airflow/hooks/base.py", line 72, in get_connection
> conn = Connection.get_connection_from_secrets(conn_id)
> File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 477, in get_connection_from_secrets
> raise AirflowNotFoundException(f"The conn_id `{conn_id}` isn't defined")
> airflow.exceptions.AirflowNotFoundException: The conn_id `` isn't defined
> {"asctime": "2024-08-05T14:56:44.959+0000", "filename": "", "lineno": 0, "levelname": "Level None", "message": "end_of_log", "exc_text": null, "offset": 1722869804959604736, "dag_id": "dag_for_testing_es_task_handler", "task_id": "task_for_testing_es_log_handler", "execution_date": "2016_01_01T00_00_00_000000", "try_number": "1", "log_id": "dag_for_testing_es_task_handler-task_for_testing_es_log_handler-2016_01_01T00_00_00_000000-1"}
> ```
This issue was related to the removed deepcopy, the deepcopy is apparently necessary for PostgresHook, so putted it back there and not in DbApiHook which fixes the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1703073923
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -66,7 +66,7 @@ versions:
dependencies:
- apache-airflow>=2.7.0
- - apache-airflow-providers-common-sql>=1.14.1
+ - apache-airflow-providers-common-sql>=1.14.2
Review Comment:
We need the versions updated as the PR was not merged in time for the current release wave
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2308779320
Looks like we need now to set min version for `apache-airflow-providers-common-sql` as 1.17.0
otherwise LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2326034038
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1696378534
##########
airflow/providers/odbc/hooks/odbc.py:
##########
@@ -99,15 +92,6 @@ def sqlalchemy_scheme(self) -> str:
raise RuntimeError("sqlalchemy_scheme in connection extra should not contain : or / characters")
return self._sqlalchemy_scheme or extra_scheme or self.DEFAULT_SQLALCHEMY_SCHEME
- @property
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1703119151
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -66,7 +66,7 @@ versions:
dependencies:
- apache-airflow>=2.7.0
- - apache-airflow-providers-common-sql>=1.14.1
+ - apache-airflow-providers-common-sql>=1.14.2
Review Comment:
Never mind I think PyCharm glitched or something
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1703118348
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -66,7 +66,7 @@ versions:
dependencies:
- apache-airflow>=2.7.0
- - apache-airflow-providers-common-sql>=1.14.1
+ - apache-airflow-providers-common-sql>=1.14.2
Review Comment:
Indeed, and while doing that I just saw there is an issue as the code isn't up-to-date with the previous changes I did that are already merged into main, so I'm a bit confused. When I compare untouched files there are differences with main but when I do a pull rebase no new commits or changes are comming in it says like up-to-date... When I do git merge main also no changes applied
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325870292
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325957958
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325720198
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #40751:
URL: https://github.com/apache/airflow/pull/40751#discussion_r1692896930
##########
airflow/providers/odbc/hooks/odbc.py:
##########
@@ -99,15 +92,6 @@ def sqlalchemy_scheme(self) -> str:
raise RuntimeError("sqlalchemy_scheme in connection extra should not contain : or / characters")
return self._sqlalchemy_scheme or extra_scheme or self.DEFAULT_SQLALCHEMY_SCHEME
- @property
Review Comment:
Those removals will require to add common.sql >= next.version in dependencies of providers that have them removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2265726757
cc: @eladkal - It would be good to include that one in the next provider's wave.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325977332
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2236306401
I think the cause of the problem is that some tests effectively started to expect the Database - likely they simply now require and use session when they did not before. The advice to set `@pytest.mark.db_test` for tests that need them is likely a better fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2230111183
@potiuk @eladkal
I've fixed the AttributeError: type object 'SkipDBTestsSession' has no attribute 'get_bind' as follow:
```
class SkipDBTestsSession:
"""
This fake session is used to skip DB tests when `_AIRFLOW_SKIP_DB_TESTS` is set.
:meta private:
"""
def __init__(self):
raise AirflowInternalRuntimeError(
"Your test accessed the DB but `_AIRFLOW_SKIP_DB_TESTS` is set.\n"
"Either make sure your test does not use database or mark the test with `@pytest.mark.db_test`\n"
"See https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#"
"best-practices-for-db-tests on how "
"to deal with it and consult examples."
)
def remove(*args, **kwargs):
pass
# added dummy get_bind method here to fix the issue
def get_bind(
self,
mapper=None,
clause=None,
bind=None,
_sa_skip_events=None,
_sa_skip_for_implicit_returning=False,
):
pass
```
I find it weird I have the same issue accross my different PR but I've checked other Pr and they don't have it...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cached property for connection_extra in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2228077844
> One [test ](https://github.com/apache/airflow/actions/runs/9935262152/job/27441226855?pr=40751) failing due to:
>
> AttributeError: type object 'SkipDBTestsSession' has no attribute 'get_bind'
Seems my other PR are also having the same issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2269311793
I'm a bit puzzled why I get following errors wiht provider compatibility tests as I didn't do any changes in Airflow itself I would assume it should be fine:
```
Traceback (most recent call last):
File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 466, in get_connection_from_secrets
conn = secrets_backend.get_connection(conn_id=conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 78, in wrapper
with create_session() as session:
File "/usr/local/lib/python3.8/contextlib.py", line 113, in __enter__
return next(self.gen)
File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 35, in create_session
raise RuntimeError("Session must be set before!")
RuntimeError: Session must be set before!
Unable to retrieve connection from secrets backend (MetastoreBackend). Checking subsequent secrets backend.
Traceback (most recent call last):
File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 466, in get_connection_from_secrets
conn = secrets_backend.get_connection(conn_id=conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 78, in wrapper
with create_session() as session:
File "/usr/local/lib/python3.8/contextlib.py", line 113, in __enter__
return next(self.gen)
File "/usr/local/lib/python3.8/site-packages/airflow/utils/session.py", line 35, in create_session
raise RuntimeError("Session must be set before!")
RuntimeError: Session must be set before!
Could not write logs to wasb://container/remote/log/location/1.log
Traceback (most recent call last):
File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/log/wasb_task_handler.py", line 210, in wasb_write
self.hook.load_string(log, self.wasb_container, remote_log_location, overwrite=True)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 360, in load_string
self.upload(
File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 417, in upload
blob_client = self._get_blob_client(container_name, blob_name)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 232, in _get_blob_client
return self.blob_service_client.get_blob_client(container=container_name, blob=blob_name)
File "/usr/local/lib/python3.8/functools.py", line 967, in __get__
val = self.func(instance)
File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 157, in blob_service_client
return self.get_conn()
File "/usr/local/lib/python3.8/site-packages/airflow/providers/microsoft/azure/hooks/wasb.py", line 161, in get_conn
conn = self.get_connection(self.conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/hooks/base.py", line 72, in get_connection
conn = Connection.get_connection_from_secrets(conn_id)
File "/usr/local/lib/python3.8/site-packages/airflow/models/connection.py", line 477, in get_connection_from_secrets
raise AirflowNotFoundException(f"The conn_id `{conn_id}` isn't defined")
airflow.exceptions.AirflowNotFoundException: The conn_id `` isn't defined
{"asctime": "2024-08-05T14:56:44.959+0000", "filename": "", "lineno": 0, "levelname": "Level None", "message": "end_of_log", "exc_text": null, "offset": 1722869804959604736, "dag_id": "dag_for_testing_es_task_handler", "task_id": "task_for_testing_es_log_handler", "execution_date": "2016_01_01T00_00_00_000000", "try_number": "1", "log_id": "dag_for_testing_es_task_handler-task_for_testing_es_log_handler-2016_01_01T00_00_00_000000-1"}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2282873923
> @eladkal Don't understand the error as we didn't modify that code:
This is because your PR modifies dependencies (which is rare) - and apparently new dependencies are breaking MyPy checks - after constraints generation happened scroll down to see the new dependencies here:
https://github.com/apache/airflow/actions/runs/10316987581
I attempt to fix it as part of https://github.com/apache/airflow/pull/41383 - because apparently lack of disks space during constraint generation in main caused constraints to not be updated during the last few days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2282874511
But compatibiliity tests are "real" - i.e. when the "main" provider tests are run against "2.7, 2.8, 2.9" - they fail because of the change in connection ids available. I suggest to check airflow version and use the connection id depending on version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2323409880
> @dabla can you make the needed change so we can merge this for next release?
Will do that tomorrow first thing in the morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325846487
> 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 @eladkal , will do that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Generalize caching of connection in DbApiHook to improve performance [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #40751:
URL: https://github.com/apache/airflow/pull/40751#issuecomment-2325882466
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org