Upgrade Elasticsearch to 8 #33135

Owen-CH-Leung · 2023-08-05T06:50:36Z

Upgrade the elasticsearch provider to use official elasticsearch package with version >8.

Remote logging with latest ES package should also work with this PR merged. To test locally, rebuild the breeze image with elasticsearch constraint set up elasticsearch>8,<9, set up elasticsearch 8 , kibana & filebeat in docker respectively, with the following changes to the airflow.cfg

[logging]
remote_logging = True

[elasticsearch]
json_format = True
host = http://host.docker.internal:9200
host_field = host.name
offset_field = log.offset

Note that with the latest es package, host field under elasticsearch has to be a full URL starting with http:// or https://. If you just put host.docker.internal:9200 that will not work

Fix failing Test Suit

potiuk · 2023-08-06T15:29:30Z

airflow/config_templates/config.yml

@@ -2379,7 +2379,7 @@ elasticsearch:
 elasticsearch_configs:
  description: ~
  options:
-    use_ssl:


Why this change?

In elasticsearch 7, use_ssl is an accepted parameter when constructing ElasticSearch client. See the following source code :

https://github.com/elastic/elasticsearch-py/blob/7.14/elasticsearch/client/__init__.py#L113

However, in elasticsearch 8, it no longer accepts use_ssl parameter. See the following source code:

https://github.com/elastic/elasticsearch-py/blob/8.9/elasticsearch/_sync/client/__init__.py#L129

Therefore to make the testsuite compile with the ES8 , I use http_compress as the argument (which is one of the accepted arguments for constructing ES client

potiuk · 2023-08-06T15:34:31Z

Looks good, but it needs a bit "surrounding changes" and "context".

I think we need to think a bit carefully about the change/migration (configs?) -and soem changelog documentation should be addded explaining what's going on and explaining context.

The Elasticsearch config can. now be moved to provider - this is where it belongs. We should keep the pre-2.7 defaults in wiki.apache.org/confluence/display/AIRFLOW/AIP-36+DAG+Versioning but the whole "elasticsearch" config should be moved to provider.yaml - same as celery, kubernetes and others.

Owen-CH-Leung · 2023-08-07T03:47:02Z

Looks good, but it needs a bit "surrounding changes" and "context".

I think we need to think a bit carefully about the change/migration (configs?) -and soem changelog documentation should be addded explaining what's going on and explaining context.

The Elasticsearch config can. now be moved to provider - this is where it belongs. We should keep the pre-2.7 defaults in wiki.apache.org/confluence/display/AIRFLOW/AIP-36+DAG+Versioning but the whole "elasticsearch" config should be moved to provider.yaml - same as celery, kubernetes and others.

Thanks. I'll some more contexts into changelog soon.

For moving elasticsearch config, do you mean the following lines should be moved to provider.yaml ?

https://github.com/apache/airflow/blob/main/airflow/config_templates/config.yml#L2396-L2410

potiuk · 2023-08-07T08:55:53Z

For moving elasticsearch config, do you mean the following lines should be moved to provider.yaml ?

https://github.com/apache/airflow/blob/main/airflow/config_templates/config.yml#L2396-L2410

Yes. See for example: #32777 - this is a new thing that has been added in Airflow 2.7 - providers can now contribute their own whole "sections" and they can be removed from generic "airflow" configuration.

Also, you will need to keep old "pre-2.7" defaults in https://github.com/apache/airflow/blob/main/airflow/config_templates/pre_2_7_defaults.cfg for back-compatibility (this one provides the default values for old providers that have not yet moved their config to ). The PR for Hive above does not have it yet as I realised it only afterwards. As you can see elasticsearch_configs is there, but I think - if we decide to also move "elasticsearch" to the provider, we need to add "elasticearch" to the defaults as well before 2.7.0 is released.

I will go ahead and add it now, otherwise some back-compatibility scenarios might not work (old provider assuming default value retrieved might stop working).

The elasticsearch group is likely to be moved to elasticsearch provider. Anticipating that (see apache#33135) we need to move it to pre-2.7 defaults in order to have back-compatibility for providers that assume default values to be there.

Owen-CH-Leung · 2023-08-07T13:15:23Z

For moving elasticsearch config, do you mean the following lines should be moved to provider.yaml ?

https://github.com/apache/airflow/blob/main/airflow/config_templates/config.yml#L2396-L2410

Yes. See for example: #32777 - this is a new thing that has been added in Airflow 2.7 - providers can now contribute their own whole "sections" and they can be removed from generic "airflow" configuration.

Also, you will need to keep old "pre-2.7" defaults in https://github.com/apache/airflow/blob/main/airflow/config_templates/pre_2_7_defaults.cfg for back-compatibility (this one provides the default values for old providers that have not yet moved their config to ). The PR for Hive above does not have it yet as I realised it only afterwards. As you can see elasticsearch_configs is there, but I think - if we decide to also move "elasticsearch" to the provider, we need to add "elasticearch" to the defaults as well before 2.7.0 is released.

I will go ahead and add it now, otherwise some back-compatibility scenarios might not work (old provider assuming default value retrieved might stop working).

Thanks. I've added some contexts in CHANGELOG and also moved the config to provider.yaml

pankajkoti · 2023-08-07T13:11:49Z

airflow/providers/elasticsearch/CHANGELOG.rst

@@ -27,6 +27,17 @@
 Changelog
 ---------

+5.1.0


Suggested change

5.1.0

6.0.0

Since we are suggesting a breaking change, we need to have a major bump

Ok - will revise accordingly

pankajkoti · 2023-08-07T13:12:31Z

airflow/providers/elasticsearch/log/es_task_handler.py

-        self.client = elasticsearch.Elasticsearch(host.split(";"), **es_kwargs)  # type: ignore[attr-defined]
-
+        self.client = elasticsearch.Elasticsearch(host, **es_kwargs)  # type: ignore[attr-defined]
+        # in airflow.cfg, host of elasticsearch has to be http://dockerhostXxxx:9200


May I know what error do we see if the protocol is not included in the set value?

In ES 8, when constructing a new ElasticSearch client, a full URL including http / https is expected, and you will get a ValueError when calling the __init__ like below:

pankajkoti · 2023-08-07T13:12:58Z

airflow/providers/elasticsearch/log/es_task_handler.py

@@ -292,27 +292,24 @@ def es_read(self, log_id: str, offset: int | str, metadata: dict) -> list | Elas
        }

        try:
-            max_log_line = self.client.count(index=self.index_patterns, body=query)["count"]
+            max_log_line = self.client.count(index=self.index_patterns, body=query)["count"]  # type: ignore


Why do we have here a type ignore?

So - if we look at the official ES package, the count API from the ElasticSearch class actually didn't have the body as parameter. See this link: https://github.com/elastic/elasticsearch-py/blob/main/elasticsearch/_sync/client/__init__.py#L801

But the body parameter is still accepted because there's a decorator at the beginning, which modifies the function to accept body as the argument. See this few lines : https://github.com/elastic/elasticsearch-py/blob/main/elasticsearch/_sync/client/__init__.py#L798-L800

Therefore, without type ignore, the pre-commit job mypy at provider will actually get failed because it thinks that body is not an accepted parameter (which actually is)

pankajkoti · 2023-08-07T13:13:13Z

airflow/providers/elasticsearch/log/es_task_handler.py


        logs: list[Any] | ElasticSearchResponse = []
        if max_log_line != 0:
            try:
                query.update({"sort": [self.offset_field]})
-                res = self.client.search(
+                res = self.client.search(  # type: ignore


same question regarding type ignore

pankajkoti · 2023-08-07T13:15:02Z

airflow/providers/elasticsearch/log/es_task_handler.py

                    index=self.index_patterns,
                    body=query,
                    size=self.MAX_LINE_PER_PAGE,
                    from_=self.MAX_LINE_PER_PAGE * self.PAGE,
                )
                logs = ElasticSearchResponse(self, res)
-            except elasticsearch.exceptions.ElasticsearchException:
-                self.log.exception("Could not read log with log_id: %s", log_id)
+            except Exception as err:


Cannot we not narrow down the exception we catch? Is the previous exception no longer present? If so, have they added any other similar class and can we use that?

Having such a broad level exception catch and not re-raising it might lead to some silent failures.

Yes the exception elasticsearch.exceptions.ElasticsearchException is no longer present. Instead new class of exceptions are defined such as UnsupportedProductError, NotFoundError and so on. See this file:

https://github.com/elastic/elasticsearch-py/blob/main/elasticsearch/exceptions.py

And I feel like all those errors can occur when calling the ES API. So maybe we should raise the exception after logging to the error log ?

pankajkoti · 2023-08-07T13:20:11Z

airflow/providers/elasticsearch/CHANGELOG.rst

+~~~~~~~~~~~~~~~~
+
+.. note::
+  Upgrade to ElasaticSearch 8. The ElasticsearchTaskHandler & ElasticsearchSQLHook will now use ElasticSearch 8 package.


Suggested change

Upgrade to ElasaticSearch 8. The ElasticsearchTaskHandler & ElasticsearchSQLHook will now use ElasticSearch 8 package.

Upgrade to Elasticsearch 8. The ElasticsearchTaskHandler & ElasticsearchSQLHook will now use Elasticsearch 8 package.

Ok. Will revise accordingly

pankajkoti · 2023-08-07T13:20:52Z

airflow/providers/elasticsearch/CHANGELOG.rst

+  This also means that the release drops support for ElasticSearch 7 and below and will no longer work with
+  ElasticSearch database that is below version 8.


Suggested change

This also means that the release drops support for ElasticSearch 7 and below and will no longer work with

ElasticSearch database that is below version 8.

This also means that the release drops support for Elasticsearch 7 and below and will no longer work with

Elasticsearch database that is below version 8.

eladkal · 2023-08-07T13:23:50Z

airflow/providers/elasticsearch/CHANGELOG.rst

+  This also means that the release drops support for ElasticSearch 7 and below and will no longer work with
+  ElasticSearch database that is below version 8.


I am not sure if this PR is a breaking change?
Airflow wise what exactly gets broken here?
Updating upstream package version by itself is not a breaking change.

Sorry - what I'm trying to say is that this release will drop support for ElasticSearch database version 7 or below since elasticsearch 8 will not work with DB version below 8 (i.e. airflow users should also upgrade their ES database to version 8 to ensure compatibility). I wrote this because I read the following doc and learn that Elasticsearch language clients are only backwards compatible with default distributions and without guarantees made

https://elasticsearch-py.readthedocs.io/en/stable/

Airflow-wise, nothing should really get broken

Yeah. @eladkal is right - dependency change is not a reason for major bump - even if it is such a "big" dependency. This very clear in https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api:

What should I do if I update my own dependencies without changing the public API?

That would be considered compatible since it does not affect the public API. Software that explicitly depends on the same dependencies as your package should have their own dependency specifications and the author will notice any conflicts. Determining whether the change is a patch level or minor level modification depends on whether you updated your dependencies in order to fix a bug or introduce new functionality. We would usually expect additional code for the latter instance, in which case it’s obviously a minor level increment.

The elasticsearch group is likely to be moved to elasticsearch provider. Anticipating that (see #33135) we need to move it to pre-2.7 defaults in order to have back-compatibility for providers that assume default values to be there.

potiuk · 2023-08-07T18:09:58Z

Thanks. I've added some contexts in CHANGELOG and also moved the config to provider.yaml

How about moving "elasticsearch" section as well? I seems very much like it should be in, this section makes no sense when elasticsearch provider is not installed I think?

potiuk · 2023-08-07T18:18:36Z

airflow/providers/elasticsearch/provider.yaml

@@ -72,3 +73,20 @@ connection-types:

 logging:
  - airflow.providers.elasticsearch.log.es_task_handler.ElasticsearchTaskHandler
+


On top of also moving [elasticsearch] section (which I thin makes sense) it should be accompanied by adding the link to elastisearch provider configuration in the Airflow documentation: (following #32777).

When we have more of those, we might want to choose to do it automatically but for now we need to add it "manually"

… add configurations-ref.rst, and revise the doc

Owen-CH-Leung · 2023-08-08T05:49:23Z

Thanks all. I've moved elasticsearch section to provider.yaml, revise the descriptions in CHANGELOG and also revised the docs. For now I keep the release version as 5.1.0 but feel free to let me know if we want to make it a major bump

potiuk · 2023-08-08T10:22:31Z

Looks cool from my side. Thanks @Owen-CH-Leung for being so responsive to our comments :)

eladkal · 2023-08-08T10:23:51Z

airflow/providers/elasticsearch/CHANGELOG.rst

+.....
+
+Misc
+~~~~


Please remove above including the version.
It is not known yet which version is going to be next.. it could be 5.0.1, 5.1.0, 6.0.0
we know the release version only in run time.

The change we need here is just the note after the changelog title.. during release the release manager set it to the proper classification

Sure. Removed.

Owen-CH-Leung · 2023-08-08T13:29:43Z

I think @pankajkoti is also testing on ES remote logging with ES 8. Let's merge the PR when he comes back to us

pankajkoti · 2023-08-08T13:59:48Z

I can confirm ES remote logging works fine for me from this branch and Elasticsearch 8.9.0 🎉 . Thanks for your patience @Owen-CH-Leung

eladkal · 2023-08-08T14:06:18Z

Cool!
Will merge when CI is green

potiuk · 2023-08-08T14:48:27Z

I can confirm ES remote logging works fine for me from this branch and Elasticsearch 8.9.0 🎉 . Thanks for your patience @Owen-CH-Leung

Fantastic news !

potiuk · 2023-08-08T16:05:53Z

aaand merged.

The elasticsearch group is likely to be moved to elasticsearch provider. Anticipating that (see #33135) we need to move it to pre-2.7 defaults in order to have back-compatibility for providers that assume default values to be there. (cherry picked from commit 2d74604)

(cherry picked from commit ad9d8d4)

Upgrade Elasticsearch to 8

5ec5a22

Fix failing Test Suit

boring-cyborg bot added area:logging area:providers provider:elasticsearch labels Aug 5, 2023

Owen-CH-Leung added 3 commits August 5, 2023 14:52

Remove debug statements

5389096

Make mypy ignore api call from es

5903216

Merge branch 'main' into Upgrade-ElasticSearch-to-8

2aca004

Owen-CH-Leung marked this pull request as ready for review August 5, 2023 09:15

eladkal requested review from potiuk and eladkal August 5, 2023 20:44

potiuk reviewed Aug 6, 2023

View reviewed changes

pankajkoti self-requested a review August 7, 2023 06:42

potiuk mentioned this pull request Aug 7, 2023

Add elasticsearch group to pre-2.7 defaults #33166

Merged

Add Changelog & move config to provider.yaml

19bca4c

pankajkoti reviewed Aug 7, 2023

View reviewed changes

eladkal requested changes Aug 7, 2023

View reviewed changes

potiuk reviewed Aug 7, 2023

View reviewed changes

Move Elasticsearch section to provider.yaml, remove breaking changes,…

54119de

… add configurations-ref.rst, and revise the doc

potiuk approved these changes Aug 8, 2023

View reviewed changes

eladkal requested changes Aug 8, 2023

View reviewed changes

Remove v5.1.0

625cc31

eladkal requested a review from pankajkoti August 8, 2023 13:35

eladkal approved these changes Aug 8, 2023

View reviewed changes

potiuk merged commit ad9d8d4 into apache:main Aug 8, 2023
61 checks passed

sunank200 mentioned this pull request Aug 10, 2023

Add backward compatibility for elasticsearch<8 #33281

Merged

eladkal mentioned this pull request Aug 25, 2023

Status of testing Providers that were prepared on August 26, 2023 #33751

Closed

63 tasks

ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 27, 2023

ephraimbuddy added this to the Airflow 2.8.0 milestone Aug 27, 2023

potiuk modified the milestones: Airflow 2.8.0, Airflow 2.7.1 Sep 1, 2023

ephraimbuddy pushed a commit that referenced this pull request Sep 2, 2023

Upgrade Elasticsearch to 8 (#33135)

de95f98

(cherry picked from commit ad9d8d4)

ephraimbuddy mentioned this pull request Sep 4, 2023

Status of testing of Apache Airflow 2.7.1rc2 #34065

Closed

97 tasks

potiuk mentioned this pull request Sep 5, 2023

Elasticsearch.__init__() got an unexpected keyword argument 'use_ssl' #34099

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Elasticsearch to 8 #33135

Upgrade Elasticsearch to 8 #33135

Owen-CH-Leung commented Aug 5, 2023 •

edited

Loading

potiuk Aug 6, 2023

Owen-CH-Leung Aug 7, 2023

potiuk commented Aug 6, 2023

Owen-CH-Leung commented Aug 7, 2023

potiuk commented Aug 7, 2023

Owen-CH-Leung commented Aug 7, 2023

pankajkoti Aug 7, 2023

Owen-CH-Leung Aug 7, 2023

pankajkoti Aug 7, 2023

Owen-CH-Leung Aug 7, 2023

pankajkoti Aug 7, 2023

Owen-CH-Leung Aug 7, 2023

pankajkoti Aug 7, 2023

pankajkoti Aug 7, 2023

Owen-CH-Leung Aug 7, 2023

pankajkoti Aug 7, 2023 •

edited

Loading

Owen-CH-Leung Aug 7, 2023

pankajkoti Aug 7, 2023

eladkal Aug 7, 2023

Owen-CH-Leung Aug 7, 2023

potiuk Aug 7, 2023

potiuk commented Aug 7, 2023

potiuk Aug 7, 2023

Owen-CH-Leung commented Aug 8, 2023

potiuk commented Aug 8, 2023

eladkal Aug 8, 2023

Owen-CH-Leung Aug 8, 2023

Owen-CH-Leung commented Aug 8, 2023

pankajkoti commented Aug 8, 2023

eladkal commented Aug 8, 2023

potiuk commented Aug 8, 2023

potiuk commented Aug 8, 2023

	Upgrade to ElasaticSearch 8. The ElasticsearchTaskHandler & ElasticsearchSQLHook will now use ElasticSearch 8 package.
	Upgrade to Elasticsearch 8. The ElasticsearchTaskHandler & ElasticsearchSQLHook will now use Elasticsearch 8 package.

		This also means that the release drops support for ElasticSearch 7 and below and will no longer work with
		ElasticSearch database that is below version 8.

		@@ -72,3 +73,20 @@ connection-types:

		logging:
		- airflow.providers.elasticsearch.log.es_task_handler.ElasticsearchTaskHandler

+              .....
+              Misc
+              ~~~~

Upgrade Elasticsearch to 8 #33135

Upgrade Elasticsearch to 8 #33135

Conversation

Owen-CH-Leung commented Aug 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Aug 6, 2023

Owen-CH-Leung commented Aug 7, 2023

potiuk commented Aug 7, 2023

Owen-CH-Leung commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pankajkoti Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Aug 7, 2023

Choose a reason for hiding this comment

Owen-CH-Leung commented Aug 8, 2023

potiuk commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Owen-CH-Leung commented Aug 8, 2023

pankajkoti commented Aug 8, 2023

eladkal commented Aug 8, 2023

potiuk commented Aug 8, 2023

potiuk commented Aug 8, 2023

Owen-CH-Leung commented Aug 5, 2023 •

edited

Loading

pankajkoti Aug 7, 2023 •

edited

Loading