Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove limitation for elasticsearch library #16553

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 21, 2021

Elasticsearch <7.6.0 does not work with Python 3.9 (import
errors on deprecated base64 functionality that have been removed
in Python 3.9) see:

ihttps://bugzilla.redhat.com/show_bug.cgi?id=1894188

This PR bumps elasticsearch library version to latest available
(7.13.1 as of this writing) in order to get it Python 3.9
compatible.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@uranusjr
Copy link
Member

Should we also change the lower bound to >=7.6.0?

setup.py Outdated
@@ -275,7 +275,7 @@ def get_sphinx_theme_version() -> str:
'pydruid>=0.4.1',
]
elasticsearch = [
'elasticsearch>7, <7.6.0',
'elasticsearch>7',
'elasticsearch-dbapi==0.1.0',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also bump up elasticsearch-dbapi? Current version is 0.2.4

Copy link
Member Author

@potiuk potiuk Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also change the lower bound to >=7.6.0?

I think we do not want to limit the lower bound in this case. I am not even sure what limit it should be to be honest :). Various problems with Python 3.9 have been solved in I think few versions between 7.5.1 and 7.13.1 (including one solved in 7.10.1) . I think we do not want to "force" people to upgrade if they don't want in this case. We could potentially add some limit for "python>=3.9" but I think this has marginal impact. Most people installing it from the scratch will have latest version and if they use constraints, even when upgrading they will bump it.

Should we also bump up elasticsearch-dbapi? Current version is 0.2.4

No idea. There is no explanation on why those limitations were added so I have no idea what effect it will have to remove it. I am summoning @jedcunningham here :)

@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

BTW. Seem that there are some unit tests that fail with this upgrade, so it's not that straightforward and we might actually have to add some upper bounds and fixes here.

@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

Which is pretty strange because those tests do not fail in local venv with the same library version :( ... looking at it.

Elasticsearch <7.6.0 does not work with Python 3.9 (import
errors on deprecated base64 functionality that have been removed
in Python 3.9) see:

ihttps://bugzilla.redhat.com/show_bug.cgi?id=1894188

This PR bumps elasticsearch library version to latest available
(7.13.1 as of this writing) in order to get it Python 3.9
compatible.
@potiuk potiuk force-pushed the remove-elasticsearch-limit-in-preparation-for-python3.9 branch from 83af63b to 6226d37 Compare June 21, 2021 10:23
@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

I removed the dbapi limits and fixed FakeElasticsearch by adding missing headers from the new version 🤞

@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

Seems like good-to-go @jedcunningham - I'd love if you take a look and see if the new version of the elasticsearch library seems OK, but it looks like at least unit tests are passing now.

@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

I also pushed now with remove dbapi fixed version 🤞

@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

Yeah. All the unit tests are passing also with elasticsearch-dbapi removal. so now I need someone who can double-check it with the "real" elasticsearch :D (@jedcunningham maybe 🙏 ?).

@jedcunningham
Copy link
Member

I'll give this a spin later today, no problem.

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested provider 2.0.1rc1, related core 2.1.1 changes, and these bumped packages and all seemed happy.

@potiuk
Copy link
Member Author

potiuk commented Jun 21, 2021

Cool. Thanks @jedcunningham. Others - I need to get this one approved to merge it and to get final - I hope - test runs for Python 3.9.

@potiuk potiuk merged commit e5e59b4 into apache:main Jun 22, 2021
@potiuk potiuk deleted the remove-elasticsearch-limit-in-preparation-for-python3.9 branch June 22, 2021 06:05
@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

one step closer to Python 3.9 :)

potiuk added a commit to potiuk/airflow that referenced this pull request Jun 22, 2021
* Remove limitation for elasticsearch library

Elasticsearch <7.6.0 does not work with Python 3.9 (import
errors on deprecated base64 functionality that have been removed
in Python 3.9) see:

ihttps://bugzilla.redhat.com/show_bug.cgi?id=1894188

This PR bumps elasticsearch library version to latest available
(7.13.1 as of this writing) in order to get it Python 3.9
compatible.

(cherry picked from commit e5e59b4)
Jorricks pushed a commit to Jorricks/airflow that referenced this pull request Jun 24, 2021
* Remove limitation for elasticsearch library

Elasticsearch <7.6.0 does not work with Python 3.9 (import
errors on deprecated base64 functionality that have been removed
in Python 3.9) see:

ihttps://bugzilla.redhat.com/show_bug.cgi?id=1894188

This PR bumps elasticsearch library version to latest available
(7.13.1 as of this writing) in order to get it Python 3.9
compatible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants