Reusable async client

[odelmarcelle]

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]

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 ⚠️
opensearchpy/connection/http_async.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dblock]

LGTM! Thanks.

Double checking that the synchronous client doesn't have this issue?

Does the client support with (too lazy to look) :))

[odelmarcelle]

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]

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]

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]

Should we be a bit defensive here and add assert _urllib3_pool_factory is not None?

[odelmarcelle]

I'm not sure to see how that would help. _urllib3_pool_factory will never be None?

[dblock]

Yes, you're right, its creation is in the constructor.

[dblock]

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