You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "sunank200 (via GitHub)" <gi...@apache.org> on 2023/08/10 09:03:23 UTC
[GitHub] [airflow] sunank200 opened a new pull request, #33281: Add backward compatibility for elasticsearch<8
sunank200 opened a new pull request, #33281:
URL: https://github.com/apache/airflow/pull/33281
<!--
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: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
For elasticsearch>8, arguments like `retry_timeout` has changed for elasticsearch to `retry_on_timeout` in `Elasticsearch()` compared to previous versions. Read more at: [documentation](https://elasticsearch-py.readthedocs.io/en/v8.8.2/api.html#module-elasticsearch) This change was done as part of the following [PR](https://github.com/apache/airflow/pull/33135).
This needs backward compatibility support for elasticsearch<8
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289849833
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@eladkal I'm not entirely sure, but the below code was part of airflow's codebase which let to the issue because of upgrade - https://github.com/apache/airflow/blob/01a6c1e7f8b0ed736d7c6f661e0c29ec023016b2/chart/values.yaml#L2149
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289864897
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@ephraimbuddy I didn't see helm tests were broken?
and to my understanding this PR brings back support for elastic search 7 so what is the motivation for the major release?
> Also, maybe we should consider bumping to major versions whenever we bump dependencies to major versions?
This was discussed and had lazy consequence if I remember correctly. Need to lookup the thread.
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289864897
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@ephraimbuddy I didn't see helm tests were broken?
and to my understanding this PR brings back support for elastic search 7 so what is the motivation for the breaking change?
> Also, maybe we should consider bumping to major versions whenever we bump dependencies to major versions?
This was discussed and had lazy consequence if I remember correctly. Need to lookup the thread.
--
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
[GitHub] [airflow] sunank200 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289809074
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
+ if retry_timeout:
+ es_kwargs["retry_on_timeout"] = retry_timeout
+ del es_kwargs["retry_timeout"]
Review Comment:
yes @utkarsharma2 I am pushing the change now
--
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
[GitHub] [airflow] sunank200 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289817905
##########
chart/values.yaml:
##########
@@ -2146,7 +2146,7 @@ config:
elasticsearch_configs:
max_retries: 3
timeout: 30
- retry_timeout: 'True'
+ retry_on_timeout: 'True'
Review Comment:
Yes, we should because in providers.yaml it points to >8 https://github.com/apache/airflow/blob/main/airflow/providers/elasticsearch/provider.yaml#L56
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289818066
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
I think it indeed HAS potential of breaking things. If we can make it "reasonably compatible" - i.e. fix back-compatibilities that we know and smooth the migration, that would be great, but I would be for marking the next ES release as MAJOR cc: @eladkal
Also there is a little twist to it. Unless I am mistaken, I think elasticsearch handler integration was anyhow broken and not working before :) . So well. it could also be seen as bugfix :P
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289807533
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
+ if retry_timeout:
+ es_kwargs["retry_on_timeout"] = retry_timeout
+ del es_kwargs["retry_timeout"]
Review Comment:
@sunank200 I think we need to be backward compatible with elasticsearch=7 as well right?
--
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
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289876708
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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
Review Comment:
if es_kwargs.get("retry_timeout"):
es_kwargs["retry_on_timeout"] = es_kwargs.pop("retry_timeout")
I think the logic should be like this
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289811472
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
cc: @Owen-CH-Leung @potiuk @eladkal WDYT?
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289859531
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
yes, I agree bumping some dependency from x.y.z to x+1.y.z may not need to be a breaking change in general. 👍🏽
However, would like to bring up and discuss the following point.
For this provider, elasticsearch is the main underlying dependency, and we're updating it to the next major version from 7.x to 8.x. It does not affect core Airflow code/functionality, but since providers are versioned and released independently, upgrading to this provider release might affect existing deployments for users.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289807533
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
+ if retry_timeout:
+ es_kwargs["retry_on_timeout"] = retry_timeout
+ del es_kwargs["retry_timeout"]
Review Comment:
@sunank200 I think we need to be backward compatible with elasticsearch=7 as well right?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289849833
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@eladkal I'm not entirely sure, but the below code was part of airflow's codebase which led to the issue because of upgrade - https://github.com/apache/airflow/blob/01a6c1e7f8b0ed736d7c6f661e0c29ec023016b2/chart/values.yaml#L2149
--
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
[GitHub] [airflow] sunank200 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289929964
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -126,6 +135,27 @@ def __init__(
self._doc_type_map: dict[Any, Any] = {}
self._doc_type: list[Any] = []
+ @staticmethod
+ def format_url(host: str) -> str:
+ """
+ Formats the given host string to ensure it starts with 'http' or 'https'.
Review Comment:
changed it: https://github.com/apache/airflow/pull/33281/commits/2984194c7f3be440831c6b92d577271c9027600b
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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
Review Comment:
changed it: https://github.com/apache/airflow/pull/33281/commits/2984194c7f3be440831c6b92d577271c9027600b
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289810072
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
IMO, https://github.com/apache/airflow/pull/33135 should be marked as a breaking change. This is one argument that we have found, but there could be more such arguments and adding back compat for them will be challenging and will be discovered only when users face issues based on the params they are using.
--
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
[GitHub] [airflow] sunank200 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289809074
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
+ if retry_timeout:
+ es_kwargs["retry_on_timeout"] = retry_timeout
+ del es_kwargs["retry_timeout"]
Review Comment:
yes @utkarsharma2 I am pushing the change now
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289859531
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
yes, I agree bumping something from x.y.z to x+1.y.z may not need to be a breaking change in general. 👍🏽
However, would like to bring up and discuss the following point.
For this provider, elasticsearch is the main underlying dependency, and we're updating it to the next major version from 7.x to 8.x. It does not affect core Airflow code/functionality, but since providers are versioned and released independently, upgrading to this provider release might affect existing deployments for users.
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
yes, I agree bumping something from x.y.z to x+1.y.z may not need to be a breaking change in general. 👍🏽
However, would like to bring up and discuss the following point.
For this provider, elasticsearch is the main underlying dependency, and we're updating it to the next major version from 7.x to 8.x. It does not affect core Airflow code/functionality, but since providers are versioned and released independently, upgrading to this provider release might affect existing deployments for users.
--
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
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289847391
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@eladkal , I think if our helm chart can't work with the new Es provider then it should be considered as breaking change.
Also, maybe we should consider bumping to major versions whenever we bump dependencies to major versions?
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289810072
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
IMO, https://github.com/apache/airflow/pull/33135 should be marked as a breaking change and released with a major bump. This is one argument that we have found, but there could be more such arguments and adding back compat for them will be challenging and will be discovered only when users face issues based on the params they are using.
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289864897
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@ephraimbuddy I didn't see helm tests were broken?
and to my understanding this PR adds support for elastic search 8 so what is the motivation for the breaking change?
> Also, maybe we should consider bumping to major versions whenever we bump dependencies to major versions?
This was discussed and had lazy consequence if I remember correctly. Need to lookup the thread.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289824114
##########
chart/values.yaml:
##########
@@ -2146,7 +2146,7 @@ config:
elasticsearch_configs:
max_retries: 3
timeout: 30
- retry_timeout: 'True'
+ retry_on_timeout: 'True'
Review Comment:
Ya, but while using someone can install the old provider 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
[GitHub] [airflow] eladkal commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289864897
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
@ephraimbuddy I didn't see helm tests were broken?
and to my understanding this PR brings back support for elastic search 7 so what is the motivation for the major release? Once this PR is merged the code is backward compatible.
> Also, maybe we should consider bumping to major versions whenever we bump dependencies to major versions?
This was discussed and had lazy consequence if I remember correctly. Need to lookup the thread.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289814292
##########
chart/values.yaml:
##########
@@ -2146,7 +2146,7 @@ config:
elasticsearch_configs:
max_retries: 3
timeout: 30
- retry_timeout: 'True'
+ retry_on_timeout: 'True'
Review Comment:
Should we change this? as it will fail for elasticsearch<=7 right?
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289818066
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
I think it indeed HAS potential of breaking things. If we can make it "reasonably compatible" - i.e. fix back-compatibilities that we know and smooth the migration, that would be great, but I would be for marking the next ES release as MAJOR regardless cc: @eladkal
Also there is a little twist to it. Unless I am mistaken, I think elasticsearch handler integration was anyhow broken and not working before :) . So well. it could also be seen as bugfix :P
--
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
[GitHub] [airflow] sunank200 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289824369
##########
chart/values.yaml:
##########
@@ -2146,7 +2146,7 @@ config:
elasticsearch_configs:
max_retries: 3
timeout: 30
- retry_timeout: 'True'
+ retry_on_timeout: 'True'
Review Comment:
reverted 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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289887724
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -126,6 +135,27 @@ def __init__(
self._doc_type_map: dict[Any, Any] = {}
self._doc_type: list[Any] = []
+ @staticmethod
+ def format_url(host: str) -> str:
+ """
+ Formats the given host string to ensure it starts with 'http' or 'https'.
Review Comment:
I think we need to rephrase this as we only try prefixing `http` and not `https` if the URL does not have a scheme.
--
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 backward compatibility for elasticsearch<8 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33281:
URL: https://github.com/apache/airflow/pull/33281#issuecomment-1816786822
I'd be for removing it. People can use previous provider versions for ES < 8 if they need
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289811472
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
cc: @Owen-CH-Leung
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
cc: @Owen-CH-Leung WDYT?
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289831004
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
Okay. yes +1 to make backward-compatible as much as possible.
I am not aware of how far it was broken. But, we have few tests and users for whom the elasticsearch handler integration was known to be working well. However, only yesterday our tests caught that the PR broke existing working setup :)
Perhaps our QA expert @vatsrahul1001 can provide more confirmation here.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289826998
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
I too think we do a major release since the underline dependency had also changed from 7.* to 8.*
--
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
[GitHub] [airflow] potiuk merged pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #33281:
URL: https://github.com/apache/airflow/pull/33281
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289814292
##########
chart/values.yaml:
##########
@@ -2146,7 +2146,7 @@ config:
elasticsearch_configs:
max_retries: 3
timeout: 30
- retry_timeout: 'True'
+ retry_on_timeout: 'True'
Review Comment:
Should we change this? as it will fail for elasticsearch<=7 right? I think we can avoid this change now that we have a 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
[GitHub] [airflow] eladkal commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289832570
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
https://github.com/apache/airflow/pull/33135 did not break any airflow code.
We agreed that bumping dependency is not a breaking change and as expected every time you bump x.y.z to x+1.y.z something can be broken for users.
I am not convencied next release should be a major one.
--
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
[GitHub] [airflow] pankajkoti commented on a diff in pull request #33281: Add backward compatibility for elasticsearch<8
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #33281:
URL: https://github.com/apache/airflow/pull/33281#discussion_r1289831004
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -98,6 +98,15 @@ def __init__(
log_id_template: str | None = None,
):
es_kwargs = es_kwargs or {}
+ # For elasticsearch>8,arguments like 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 es_kwargs:
+ retry_timeout = es_kwargs.get("retry_timeout")
Review Comment:
Okay.
I am not aware of how far it was broken. But, we have few tests and users for whom the elasticsearch handler integration was known to be working well. However, only yesterday our tests caught that the PR broke existing working setup :)
Perhaps our QA expert @vatsrahul1001 can provide more confirmation here.
--
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 backward compatibility for elasticsearch<8 [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #33281:
URL: https://github.com/apache/airflow/pull/33281#issuecomment-1826401956
https://github.com/apache/airflow/pull/35707 raised to remove the functionality added by this PR
--
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 backward compatibility for elasticsearch<8 [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #33281:
URL: https://github.com/apache/airflow/pull/33281#issuecomment-1816741321
I may be missing something here but...
This PR added backward compatibility for `elasticsearch<8`
yet min version is 8.10
https://github.com/apache/airflow/blob/064fc2b7751a44e37ccce97609cff7c496098e56/generated/provider_dependencies.json#L375
so either we need to relax the min version or we should remove the backward compatibility code cc @dstandish
--
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