Skip to content

Commit

Permalink
Fixed retries (#55)
Browse files Browse the repository at this point in the history
* Fixed retries

We weren't retrying, since we didn't specify which error codes to retry
on.

This fixes that, and includes tests (which I should have added in the
first place). We use `responses` to mock the response to requests.
  • Loading branch information
Tom Augspurger authored Apr 14, 2023
1 parent 7e2b9f6 commit 359b6a1
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 4 deletions.
6 changes: 4 additions & 2 deletions planetary_computer/sas.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ class SASToken(SASBase):

def sign(self, href: str) -> SignedLink:
"""Signs an href with this token"""
return SignedLink(href=f"{href}?{self.token}", expiry=self.expiry)
return SignedLink(
href=f"{href}?{self.token}", expiry=self.expiry # type: ignore [call-arg]
)

def ttl(self) -> float:
"""Number of seconds the token is still valid for"""
Expand Down Expand Up @@ -264,7 +266,6 @@ def _sign_fsspec_asset_in_place(asset: AssetLike) -> None:

storage_options = None
for key in ["table:storage_options", "xarray:storage_options"]:

if key in extra_d:
storage_options = extra_d[key]
break
Expand Down Expand Up @@ -444,6 +445,7 @@ def get_token(
retry = urllib3.util.retry.Retry(
total=retry_total,
backoff_factor=retry_backoff_factor,
status_forcelist=[429, 500, 502, 503, 504],
)
adapter = requests.adapters.HTTPAdapter(max_retries=retry)
session.mount("http://", adapter)
Expand Down
3 changes: 1 addition & 2 deletions scripts/cibuild
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
else
# Install/upgrade dependencies
python -m pip install --upgrade pip
pip install -r requirements-dev.txt
pip install -e .[adlfs,azure]
pip install -e .[adlfs,azure,dev]

./scripts/test
fi
Expand Down
8 changes: 8 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ install_requires =
[options.extras_require]
adlfs = adlfs
azure = azure-storage-blob
dev =
black
flake8
mypy
types-requests
setuptools
pytest
responses

[options.entry_points]
console_scripts =
Expand Down
19 changes: 19 additions & 0 deletions tests/test_signing.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
from pathlib import Path
import warnings
import pystac
import pytest
from requests.exceptions import RetryError

import responses
import requests

import planetary_computer as pc
Expand Down Expand Up @@ -367,3 +370,19 @@ def test_is_fsspec_url(self) -> None:

asset = Asset("adlfs://my-container/my/path.ext")
self.assertFalse(is_fsspec_asset(asset))


@responses.activate
def test_retry() -> None:
TOKEN_CACHE.clear()
rsp1 = responses.Response(
method="GET",
url="https://planetarycomputer.microsoft.com/api/sas/v1/token/naipeuwest/naip",
status=503,
)
responses.add(rsp1)

with pytest.raises(RetryError):
get_token("naipeuwest", "naip")

assert rsp1.call_count == 11

0 comments on commit 359b6a1

Please sign in to comment.