You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Owen-CH-Leung (via GitHub)" <gi...@apache.org> on 2024/08/30 02:13:01 UTC
[PR] Fix ElasticSearch SQLClient deprecation warning [airflow]
Owen-CH-Leung opened a new pull request, #41871:
URL: https://github.com/apache/airflow/pull/41871
After the merge of this [PR](https://github.com/apache/airflow/pull/41537), the import of `SQLClient` has resulted in a deprecation warning (when being used for remote logging) :
https://github.com/elastic/elasticsearch-py/blob/4ee46509e236bf30f540d173b48bb7650159556d/elasticsearch/client.py#L75-L80
So this PR fixes the warning by using the SQL Interface inside a elasaticsearch client (So importing SQLClient is no longer needed)
--
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] Fix ElasticSearch SQLClient deprecation warning [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #41871:
URL: https://github.com/apache/airflow/pull/41871
--
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] Fix ElasticSearch SQLClient deprecation warning [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41871:
URL: https://github.com/apache/airflow/pull/41871#discussion_r1738712943
##########
airflow/providers/elasticsearch/hooks/elasticsearch.py:
##########
@@ -34,6 +34,9 @@
from airflow.models.connection import Connection as AirflowConnection
+if "pytest" in sys.modules:
+ import elasticsearch # noqa: F401
Review Comment:
This sounds very strange and I would never expect to check in "prod" code whether pytest is in modules. That is very suspicious - what happens if someone does have pytest in their dependencies (for whatever reason?) I do not understand why this one would be needed here. It's also under TYPE_CHECKIN Why do you need to check for pytest here?
IMHO. Way better solution will be to make "from elasticsearch import ElasticSearch" local in all the methods that need it and leave import elasticsearch unconditionally in TYPE_CHECKING.
--
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] Fix ElasticSearch SQLClient deprecation warning [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #41871:
URL: https://github.com/apache/airflow/pull/41871#discussion_r1738826990
##########
airflow/providers/elasticsearch/hooks/elasticsearch.py:
##########
@@ -34,6 +34,9 @@
from airflow.models.connection import Connection as AirflowConnection
+if "pytest" in sys.modules:
+ import elasticsearch # noqa: F401
Review Comment:
Thanks. Yes indeed this doesn't look good. I was doing it originally for unittest patch to be easier.
I've removed it and rewrite the test to patch on `elasticsearch.Elasticsearch` directly.
--
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