Basic ElasticSearch / ElasticClient 8.x Support #8519
Conversation
There was a problem hiding this comment.
also you need to relax the version support here https://github.com/celery/celery/blob/main/requirements/extras/elasticsearch.txt and possibly adjust some test.
celery/backends/elasticsearch.py
Outdated
| scheme=self.scheme, | ||
| http_auth=http_auth, | ||
| ) | ||
| if elasticsearch.VERSION[0] <= 7: |
There was a problem hiding this comment.
I'll do some digging on the client side to see what specific version they removed the scheme parameter from. But I know the 8.9 client version I installed did not have it where the 7.17 client did. So I assumed that 7.x and below would require the scheme parameter. I'll also check if I supply it as part of the URI with the 7.x client if I can skip the scheme entirely and not have that if check at all.
There was a problem hiding this comment.
we can try keep support of both 7.x and 8.x for better upgrade ability
removed if check for clientside version 7.x or below as specifying the scheme in the URI works like it does with the 8.x client.
Support up to 8.9 python client. elasticsearch 7.x client does not work with elasticsearch 8.x server and vice versa.
|
we might need to pull 99b000d from main branch |
|
The following tests are also failing so adjusting or fixing them will be needed =========================== short test summary info ============================ |
|
This is for the client library isn’t it? Latest version reported when installed is 8.9(the one I’m using).
I clearly could be misunderstanding. 😊
From: Asif Saif Uddin ***@***.***>
Date: Wednesday, September 20, 2023 at 11:29 PM
To: celery/celery ***@***.***>
Cc: Oberdorf, Justin ***@***.***>, Author ***@***.***>
Subject: Re: [celery/celery] Basic ElasticSearch / ElasticClient 8.x Support (PR #8519)
@auvipy requested changes on this pull request.
…________________________________
In requirements/extras/elasticsearch.txt<#8519 (comment)>:
@@ -1 +1 @@
-elasticsearch<8.0
+elasticsearch<=8.9
⬇️ Suggested change
-elasticsearch<=8.9
+elasticsearch<=8.1.0
—
Reply to this email directly, view it on GitHub<#8519 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2B7IQD4IHD4DY6RR63JN5DX3OYAPANCNFSM6AAAAAA46OQSE4>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
________________________________________
IMPORTANT - PLEASE READ: This electronic message, including its attachments, is CONFIDENTIAL and may contain PROPRIETARY or LEGALLY PRIVILEGED or PROTECTED information and is intended for the authorized recipient of the sender. If you are not the intended recipient, you are hereby notified that any use, disclosure, copying, or distribution of this message or any of the information included in it is unauthorized and strictly prohibited. If you have received this message in error, please immediately notify the sender by reply e-mail and permanently delete this message and its attachments, along with any copies thereof, from all locations received (e.g., computer, mobile device, etc.). To the extent permitted by law, we may monitor electronic communications for the purposes of ensuring compliance with our legal and regulatory obligations and internal policies. We may also collect email traffic headers for analyzing patterns of network traffic and managing client relationships. For further information see our privacy-policy<https://www.iqvia.com/about-us/privacy/privacy-policy>. Thank you.
|
|
Yes I was also trying to see if the tests were passing by lowering the version a bit. We can first try to make 8.0.x work then we can go for 8.9 step by step. But im open to your approach as well |
|
That works for me, I’ll adjust it.
From: Asif Saif Uddin ***@***.***>
Date: Thursday, September 21, 2023 at 9:09 AM
To: celery/celery ***@***.***>
Cc: Oberdorf, Justin ***@***.***>, Author ***@***.***>
Subject: Re: [celery/celery] Basic ElasticSearch / ElasticClient 8.x Support (PR #8519)
Yes I was also trying to see if the tests were passing by lowering the version a bit. We can first try to make 8.0.x work then we can go for 8.9 step by step. But im open to your approach as well
—
Reply to this email directly, view it on GitHub<#8519 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2B7IQFTX3JDFE3ZYXHAJBLX3Q4BTANCNFSM6AAAAAA46OQSE4>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
…________________________________________
IMPORTANT - PLEASE READ: This electronic message, including its attachments, is CONFIDENTIAL and may contain PROPRIETARY or LEGALLY PRIVILEGED or PROTECTED information and is intended for the authorized recipient of the sender. If you are not the intended recipient, you are hereby notified that any use, disclosure, copying, or distribution of this message or any of the information included in it is unauthorized and strictly prohibited. If you have received this message in error, please immediately notify the sender by reply e-mail and permanently delete this message and its attachments, along with any copies thereof, from all locations received (e.g., computer, mobile device, etc.). To the extent permitted by law, we may monitor electronic communications for the purposes of ensuring compliance with our legal and regulatory obligations and internal policies. We may also collect email traffic headers for analyzing patterns of network traffic and managing client relationships. For further information see our privacy-policy<https://www.iqvia.com/about-us/privacy/privacy-policy>. Thank you.
|
|
sadly all the unit tests are failing so we need to update/adjust the tests as well related to the result back end |
|
I’ll try and look at this today. I gave it a cursory glance yesterday, and the tests pull in the elasticsearch 8.x client, we need to remove all traces of doc_type from the tests as it’s not used. Same goes for the “scheme” parameter when instantiating the client.
From: Asif Saif Uddin ***@***.***>
Date: Friday, September 22, 2023 at 8:41 AM
To: celery/celery ***@***.***>
Cc: Oberdorf, Justin ***@***.***>, Author ***@***.***>
Subject: Re: [celery/celery] Basic ElasticSearch / ElasticClient 8.x Support (PR #8519)
sadly all the unit tests are failing so we need to update/adjust the tests as well related to the result back end
—
Reply to this email directly, view it on GitHub<#8519 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2B7IQDH7VY74HDN4ZUS243X3WBQ3ANCNFSM6AAAAAA46OQSE4>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
…________________________________________
IMPORTANT - PLEASE READ: This electronic message, including its attachments, is CONFIDENTIAL and may contain PROPRIETARY or LEGALLY PRIVILEGED or PROTECTED information and is intended for the authorized recipient of the sender. If you are not the intended recipient, you are hereby notified that any use, disclosure, copying, or distribution of this message or any of the information included in it is unauthorized and strictly prohibited. If you have received this message in error, please immediately notify the sender by reply e-mail and permanently delete this message and its attachments, along with any copies thereof, from all locations received (e.g., computer, mobile device, etc.). To the extent permitted by law, we may monitor electronic communications for the purposes of ensuring compliance with our legal and regulatory obligations and internal policies. We may also collect email traffic headers for analyzing patterns of network traffic and managing client relationships. For further information see our privacy-policy<https://www.iqvia.com/about-us/privacy/privacy-policy>. Thank you.
|
|
I fixed the tests, please take a look and see if it's to everyone/your satisfaction. Also let me know if we need to pull this in:
|
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8519 +/- ##
=======================================
Coverage 87.45% 87.46%
=======================================
Files 148 148
Lines 18499 18511 +12
Branches 3158 3163 +5
=======================================
+ Hits 16179 16191 +12
Misses 2032 2032
Partials 288 288
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
It's better to rebase on You can run |
|
I think see should check if the test coverage can be improve for the PR. others looks okish |
Description
Adds necessary changes to make elasticsearch 8.x backend's work along with elasticsearch python client 8.x
Keep in mind you want to use an 8.x client library to connect to an 8.x server