Taragolis commented on code in PR #35707:
URL: https://github.com/apache/airflow/pull/35707#discussion_r1407281095
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -70,17 +70,6 @@ def get_es_kwargs_from_config() -> dict[str, Any]:
if elastic_search_config
else {}
)
- # For elasticsearch>8 retry_timeout have changed for elasticsearch to
retry_on_timeout
- # in Elasticsearch() compared to previous versions.
- # Read more at:
https://elasticsearch-py.readthedocs.io/en/v8.8.2/api.html#module-elasticsearch
- if (
- elastic_search_config
- and "retry_timeout" in elastic_search_config
- and not kwargs_dict.get("retry_on_timeout")
- ):
- retry_timeout = elastic_search_config.get("retry_timeout")
- if retry_timeout is not None:
- kwargs_dict["retry_on_timeout"] = retry_timeout
Review Comment:
I think this for backcompat with previous config.
The problem here that we never raise Depreciation Warning here and just
silently replace `retry_timeout` to `retry_on_timeout`
In theory it could be breaking changes in the other hand note to 5.0.0
release could cover this change
https://airflow.apache.org/docs/apache-airflow-providers-elasticsearch/stable/changelog.html#id9
##########
tests/providers/elasticsearch/log/test_es_task_handler.py:
##########
@@ -671,14 +671,11 @@ def test_retrieve_config_keys():
"""
with conf_vars(
{
- ("elasticsearch_configs", "use_ssl"): "True",
Review Comment:
I think we need to keep it in the tests for check that we do not use
non-existed parameters
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
