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

fix retries and refactor httpsession #877

Merged
merged 14 commits into from
Aug 20, 2021
Merged

fix retries and refactor httpsession #877

merged 14 commits into from
Aug 20, 2021

Conversation

thehesiod
Copy link
Collaborator

@thehesiod thehesiod commented Aug 20, 2021

supposedly I can remove the identity fix given we now disable compression, we shall see if it works

with freshening up of links and making API more in-line with botocore

@thehesiod thehesiod linked an issue Aug 20, 2021 that may be closed by this pull request
6 tasks
@thehesiod
Copy link
Collaborator Author

adding reviews in case you guys are free, otherwise will merge after some more local testing

@thehesiod
Copy link
Collaborator Author

actually reading my own bug: boto/botocore#1255 looks like we can't, will put back

@aio-libs aio-libs deleted a comment from lgtm-com bot Aug 20, 2021
CHANGES.rst Outdated Show resolved Hide resolved
@thehesiod
Copy link
Collaborator Author

ooo, we'll need a minor bump because the exceptions raised will be different

@mariaines
Copy link

mariaines commented Aug 20, 2021

Hmm, I am getting SSLError:

SSL validation failed for https://dynamodb.us-west-2.amazonaws.com/ Cannot connect to host dynamodb.us-west-2.amazonaws.com:443 ssl:True [SSLCertVerificationError: (1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1131)')]

No changes except updating aiobotocore to this branch, boto3 to 1.17.106, and botocore to 1.20.106:
Screen Shot 2021-08-19 at 9 58 57 PM

@mariaines
Copy link

Might be boto/boto3#2733, let me see if I can get around it

@aio-libs aio-libs deleted a comment from lgtm-com bot Aug 20, 2021
@thehesiod
Copy link
Collaborator Author

@mariaines trying locally

@thehesiod
Copy link
Collaborator Author

fyi on gitter if you need quick resolution

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2021

This pull request introduces 2 alerts when merging 7cb0bde into 247b7ac - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@thehesiod
Copy link
Collaborator Author

repro'd, investigating

@mariaines
Copy link

Same problem when I try pinning certifi==2020.11.8 or even certifi==2018.4.16 (as suggested here) so I guess maybe not boto/boto3#2733

No rush, mostly just curious to get this working. Appreciate the investigation!

@thehesiod
Copy link
Collaborator Author

fun-fun, just pushed up fix

@mariaines
Copy link

woot seems to be working! Thank you for the quick fixes!

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2021

This pull request introduces 2 alerts when merging 5d53c5c into 247b7ac - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2021

This pull request introduces 2 alerts when merging c9461ce into 247b7ac - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #877 (96ccb63) into master (d3db4f3) will increase coverage by 0.79%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
+ Coverage   89.87%   90.67%   +0.79%     
==========================================
  Files          40       41       +1     
  Lines        4376     4440      +64     
==========================================
+ Hits         3933     4026      +93     
+ Misses        443      414      -29     
Flag Coverage Δ
unittests 90.67% <77.77%> (+0.79%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/botocore/test_signers.py 100.00% <ø> (ø)
tests/test_version.py 97.50% <ø> (ø)
tests/conftest.py 91.22% <50.00%> (ø)
aiobotocore/httpsession.py 74.00% <74.00%> (ø)
aiobotocore/__init__.py 100.00% <100.00%> (ø)
aiobotocore/endpoint.py 93.54% <100.00%> (+7.58%) ⬆️
aiobotocore/waiter.py 90.38% <100.00%> (ø)
tests/test_basic_s3.py 84.80% <100.00%> (ø)
tests/test_patches.py 95.83% <100.00%> (+0.08%) ⬆️
tests/test_session.py 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3db4f3...96ccb63. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2021

This pull request introduces 2 alerts when merging f521ffe into 247b7ac - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2021

This pull request introduces 2 alerts when merging 96ccb63 into d3db4f3 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@thehesiod thehesiod merged commit ef96004 into master Aug 20, 2021
@thehesiod thehesiod deleted the thehesiod/retry-fix branch August 20, 2021 10:09
@thehesiod
Copy link
Collaborator Author

to all those following this, you now have to set the AIOBOTOCORE_DEPRECATED_1_4_0_APIS env var for the retries to fix

jacobtomlinson pushed a commit to dask/dask-cloudprovider that referenced this pull request Aug 25, 2021
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.

No retries for ServerTimeoutError
2 participants