opensearch-project / opensearch-py

Python Client for OpenSearch
https://opensearch.org/docs/latest/clients/python/
Apache License 2.0
359 stars 178 forks source link

Reusable async client #639

Closed odelmarcelle closed 10 months ago

odelmarcelle commented 11 months ago

Description

Clears out the session attribute of AsyncHttpConnection and AIOHttpConnection when calling close(), which allow re-using a previously closed AsyncOpensearch instance.

Issues Resolved

Closes #637

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov[bot] commented 11 months ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (e323ab2) 72.12% compared to head (d4edaa1) 72.10%. Report is 1 commits behind head on main.

Files Patch % Lines
opensearchpy/connection/http_urllib3.py 60.00% 4 Missing :warning:
opensearchpy/connection/http_async.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #639 +/- ## ========================================== - Coverage 72.12% 72.10% -0.02% ========================================== Files 89 89 Lines 7939 7949 +10 ========================================== + Hits 5726 5732 +6 - Misses 2213 2217 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odelmarcelle commented 11 months ago

Turns out the issue affects clients using Urllib3HttpConnection which discard the pool, but RequestsHttpConnection works fine with my local testing.

I discovered this by adding test for the synchronous client, which apparently uses Urllib3HttpConnection. I'm not too sure where to put a test that would use RequestsHttpConnection.

saimedhi commented 11 months ago

Turns out the issue affects clients using Urllib3HttpConnection which discard the pool, but RequestsHttpConnection works fine with my local testing.

I discovered this by adding test for the synchronous client, which apparently uses Urllib3HttpConnection. I'm not too sure where to put a test that would use RequestsHttpConnection.

Hello @odelmarcelle , please put your test here for RequestsHttpConnection. And also DCO is failing. Please fix it. Thank you.

odelmarcelle commented 11 months ago

Resolved the DCO.

@saimedhi AFAIK, the test won't work there as there's not connection established to an opensearch instance. It should be somewhere under /test_server/, right? I didn't manage to work it out by myself.

One last comment though: Why are there two async connection classes defined? I'm not too sure to understand why AsyncHttpConnection and AIOHttpConnection co-exist. Especially as the former is the one recommended in the guides while most tests are based on the latter.

dblock commented 10 months ago

LMK if you prefer to merge as is, or if you think adding that assert is a good idea.