Repository: planetlabs/planet-client-python

Issue:
implement retry with exponential backoff for TOO_MANY_REQUESTS
In #276, it was discovered that httpx retry is not for status codes but instead for connection errors. Therefore, retry with [exponential backoff](https://en.wikipedia.org/wiki/Exponential_backoff) for TOO_MANY_REQUESTS needs to be implemented in our codebase.

This will fix #349 

Discussion:

Patch:
diff --git a/planet/cli/orders.py b/planet/cli/orders.py
index 5d4004c..efb2912 100644
--- a/planet/cli/orders.py
+++ b/planet/cli/orders.py
@@ -182,10 +182,6 @@ async def wait(ctx, order_id, delay, max_attempts, state):
 @translate_exceptions
 @coro
 @click.argument('order_id', type=click.UUID)
-@click.option('--checksum',
-              default=None,
-              type=click.Choice(['MD5', 'SHA256'], case_sensitive=False),
-              help=('Verify that checksums match.'))
 @click.option('--directory',
               default='.',
               help=('Base directory for file download.'),
@@ -197,18 +193,14 @@ async def wait(ctx, order_id, delay, max_attempts, state):
               is_flag=True,
               default=False,
               help=('Overwrite files if they already exist.'))
-async def download(ctx, order_id, overwrite, directory, checksum):
-    """Download order by order ID.
-
-If --checksum is provided, the associated checksums given in the manifest
-are compared against the downloaded files to verify that they match."""
+async def download(ctx, order_id, overwrite, directory):
+    """Download order by order ID."""
     quiet = ctx.obj['QUIET']
     async with orders_client(ctx) as cl:
         await cl.download_order(str(order_id),
                                 directory=directory,
                                 overwrite=overwrite,
-                                progress_bar=not quiet,
-                                checksum=checksum)
+                                progress_bar=not quiet)
 
 
 def read_file_geojson(ctx, param, value):
diff --git a/planet/clients/orders.py b/planet/clients/orders.py
index 6b8c7c8..812adcc 100644
--- a/planet/clients/orders.py
+++ b/planet/clients/orders.py
@@ -18,10 +18,7 @@ import os
 import time
 import typing
 import uuid
-import json
-import hashlib
 
-from pathlib import Path
 from .. import exceptions
 from ..constants import PLANET_BASE_URL
 from ..http import Session
@@ -272,68 +269,18 @@ class OrdersClient:
                              progress_bar=progress_bar)
         return dl_path
 
-    @staticmethod
-    def _validate_checksum(manifest_data: dict, filenames: list,
-                           checksum: str):
-        """Calculate checksum and validate that it passes.
-
-        Parameters:
-            manifest_data: The contents of the opened manifest.json file.
-            filenames: List of downloaded assets.
-            checksum: The type of checksum hash- 'MD5' or 'SHA256'.
-
-        Returns:
-            Nothing if checksums were succesful.
-
-        Raises:
-            planet.exceptions.ClientError: If the checksum fails."""
-
-        # Save hashkey & respective filename from manifest json into dictionary
-        # This is the origin hashkey
-        if checksum == 'MD5':
-            hash_type = hashlib.md5
-            checksum = checksum.lower()
-        elif checksum == 'SHA256':
-            hash_type = hashlib.sha256
-            checksum = checksum.lower()
-        file_key_pairs = {}
-        for json_entry in manifest_data['files']:
-            file_name = Path(json_entry['path']).name
-            origin_hash = json_entry['digests'][checksum]
-            file_key_pairs[file_name] = origin_hash
-        # For each downloaded file, retrieve origin hashkey from dict
-        filenames_loop = [
-            x for x in filenames if not x.endswith('manifest.json')
-        ]
-        for filename in filenames_loop:
-            downloaded_file_name = Path(filename).name
-            origin_hash = file_key_pairs[downloaded_file_name]
-            # For each file (not including manifest json),
-            # calculate hash on contents. This is the returned hash.
-            with open(filename, 'rb') as file_to_check:
-                json_data = file_to_check.read()
-                returned_hash = hash_type(json_data).hexdigest()
-                # Compare original hashkey in dict with calculated
-                if origin_hash != returned_hash:
-                    raise exceptions.ClientError(
-                        'Checksum failed. File ({filename}) not correctly \
-                        downloaded.')
-        return None
-
     async def download_order(self,
                              order_id: str,
                              directory: str = None,
                              overwrite: bool = False,
-                             progress_bar: bool = False,
-                             checksum: str = None) -> typing.List[str]:
+                             progress_bar: bool = False) -> typing.List[str]:
         """Download all assets in an order.
 
         Parameters:
-            order_id: The ID of the order.
-            directory: Base directory for file download.
+            order_id: The ID of the order
+            directory: Root directory for file download.
             overwrite: Overwrite files if they already exist.
             progress_bar: Show progress bar during download.
-            checksum: The type of checksum hash- MD5 or SHA256.
 
         Returns:
             Paths to downloaded files.
@@ -342,9 +289,6 @@ class OrdersClient:
             planet.exceptions.APIError: On API error.
             planet.exceptions.ClientError: If the order is not in a final
                 state.
-            planet.exceptions.ClientError: If more than one manifest file
-                per order.
-            planet.exceptions.ClientError: If no manifest file found in order.
         """
         order = await self.get_order(order_id)
         order_state = order['state']
@@ -365,24 +309,6 @@ class OrdersClient:
                                       progress_bar=progress_bar)
             for location in locations
         ]
-        if checksum:
-            # Checksum Implementation
-            manifest_files = [
-                x for x in filenames if Path(x).name == 'manifest.json'
-            ]
-            manifest_count = len(manifest_files)
-            if manifest_count > 1:
-                raise exceptions.ClientError(
-                    'Only 1 manifest.json expected per order.\
-                                             Recieved: {manifest_count}')
-            elif manifest_count is None:
-                raise exceptions.ClientError('No manifest file found.')
-            manifest_json = manifest_files[0]
-            with open(manifest_json, 'rb') as manifest:
-                manifest_data = json.load(manifest)
-                self._validate_checksum(manifest_data=manifest_data,
-                                        filenames=filenames,
-                                        checksum=checksum)
         return filenames
 
     @staticmethod
diff --git a/planet/http.py b/planet/http.py
index 319951f..6c829df 100644
--- a/planet/http.py
+++ b/planet/http.py
@@ -16,6 +16,7 @@ from __future__ import annotations  # https://stackoverflow.com/a/33533514
 import asyncio
 from http import HTTPStatus
 import logging
+import random
 
 import httpx
 
@@ -23,8 +24,8 @@ from .auth import Auth, AuthType
 from . import exceptions, models
 from .__version__ import __version__
 
-RETRY_COUNT = 5
-RETRY_WAIT_TIME = 1  # seconds
+MAX_RETRIES = 5
+MAX_RETRY_BACKOFF = 64  # seconds
 
 LOGGER = logging.getLogger(__name__)
 
@@ -136,8 +137,9 @@ class Session(BaseSession):
         self._client.event_hooks['response'] = [
             alog_response, araise_for_status
         ]
-        self.retry_wait_time = RETRY_WAIT_TIME
-        self.retry_count = RETRY_COUNT
+
+        self.max_retries = MAX_RETRIES
+        self.max_retry_backoff = MAX_RETRY_BACKOFF
 
     async def __aenter__(self):
         return self
@@ -151,18 +153,19 @@ class Session(BaseSession):
     async def _retry(self, func, *a, **kw):
         """Run an asynchronous request function with retry.
 
+        Retry uses exponential backoff
+
         Raises:
             planet.exceptions.TooManyRequests: When retry limit is exceeded.
         """
-        # TODO: retry will be provided in httpx v1 [1] with usage [2]
-        # 1. https://github.com/encode/httpcore/pull/221
-        # 2. https://github.com/encode/httpx/blob/
-        # 89fb0cbc69ea07b123dd7b36dc1ed9151c5d398f/docs/async.md#explicit-transport-instances # noqa
+        # NOTE: if we need something more fancy, consider the following libs:
+        # * https://pypi.org/project/retry/
+        # * https://pypi.org/project/retrying/
+        # TODO: consider increasing the scope of retryable exceptions, ex:
+        # https://github.com/googleapis/python-storage/blob/1dc6d647b86466bf133
+        # fe03ec8d76c541e19c632/google/cloud/storage/retry.py#L23-L30
         # TODO: if throttling is necessary, check out [1] once v1
         # 1. https://github.com/encode/httpx/issues/984
-        retry_count = self.retry_count
-        wait_time = self.retry_wait_time
-
         num_tries = 0
         while True:
             num_tries += 1
@@ -170,16 +173,37 @@ class Session(BaseSession):
                 resp = await func(*a, **kw)
                 break
             except exceptions.TooManyRequests as e:
-                if num_tries > retry_count:
+                if num_tries > self.max_retries:
                     raise e
                 else:
                     LOGGER.debug(f'Try {num_tries}')
+                    wait_time = self._calculate_wait(num_tries,
+                                                     self.max_retry_backoff)
                     LOGGER.info(f'Too Many Requests: sleeping {wait_time}s')
-                    # TODO: consider exponential backoff
-                    # https://developers.planet.com/docs/data/api-mechanics/
                     await asyncio.sleep(wait_time)
         return resp
 
+    @staticmethod
+    def _calculate_wait(num_tries, max_retry_backoff):
+        """Calculates retry wait
+
+        Base wait period is calculated as a exponential based on the number of
+        tries. Then, a random jitter of up to 1s is added to the base wait
+        to avoid waves of requests in the case of multiple requests. Finally,
+        the wait is thresholded to the maximum retry backoff.
+
+        Because threshold is applied after jitter, calculations that hit
+        threshold will not have random jitter applied, they will simply result
+        in the threshold value being returned.
+
+        Ref:
+        * https://developers.planet.com/docs/data/api-mechanics/
+        * https://cloud.google.com/iot/docs/how-tos/exponential-backoff
+        """
+        random_number_milliseconds = random.randint(0, 1000) / 1000.0
+        calc_wait = 2**num_tries + random_number_milliseconds
+        return min(calc_wait, max_retry_backoff)
+
     async def request(self,
                       request: models.Request,
                       stream: bool = False) -> models.Response:


Test patch:
diff --git a/tests/integration/test_orders_api.py b/tests/integration/test_orders_api.py
index bf8bd16..24d0ed3 100644
--- a/tests/integration/test_orders_api.py
+++ b/tests/integration/test_orders_api.py
@@ -19,7 +19,6 @@ import math
 import os
 from pathlib import Path
 from unittest.mock import call, create_autospec
-import hashlib
 
 import httpx
 import pytest
@@ -548,171 +547,6 @@ async def test_download_asset_md(tmpdir, session):
     assert Path(filename).name == 'metadata.json'
 
 
-@respx.mock
-@pytest.mark.asyncio
-@pytest.mark.parametrize("checksum", [("MD5"), ("SHA256")])
-async def test_checksum_success(tmpdir,
-                                order_description,
-                                oid,
-                                session,
-                                checksum):
-    # Mock an HTTP response for download
-    order_description['state'] = 'success'
-    dl_url1 = TEST_DOWNLOAD_URL + '/asset1'
-    dl_url2 = TEST_DOWNLOAD_URL + '/asset2'
-    dl_url3 = TEST_DOWNLOAD_URL + '/manifest'
-    order_description['_links']['results'] = [{
-        'location': dl_url1
-    }, {
-        'location': dl_url2
-    }, {
-        'location': dl_url3
-    }]
-    get_url = f'{TEST_ORDERS_URL}/{oid}'
-    get_order_response = httpx.Response(HTTPStatus.OK, json=order_description)
-    respx.get(get_url).return_value = get_order_response
-
-    # The first asset in the order:
-    # A 1-byte binary file containing the value "1".
-    asset1_content = b"1"
-    asset1_response = httpx.Response(HTTPStatus.OK,
-                                     content=asset1_content,
-                                     headers={
-                                         'Content-Type':
-                                         'image/tiff',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset1.tif"'
-                                     })
-    respx.get(dl_url1).return_value = asset1_response
-
-    asset2_response = httpx.Response(HTTPStatus.OK,
-                                     json={'foo': 'bar'},
-                                     headers={
-                                         'Content-Type':
-                                         'application/json',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset2.json"'
-                                     })
-    respx.get(dl_url2).return_value = asset2_response
-    # Create a mock manifest which has the correct hashkeys for each asset
-    manifest = httpx.Response(
-        HTTPStatus.OK,
-        json={
-            "name":
-            "",
-            "files":
-            [{
-                "path": "asset1.tif",
-                "digests": {
-                    "md5": hashlib.md5(asset1_response.content).hexdigest(),
-                    "sha256":
-                    hashlib.sha256(asset1_response.content).hexdigest()
-                },
-            },
-             {
-                 "path": "asset2.json",
-                 "digests": {
-                     "md5": hashlib.md5(asset2_response.content).hexdigest(),
-                     "sha256":
-                     hashlib.sha256(asset2_response.content).hexdigest()
-                 }
-             }]
-        },
-        headers={
-            'Content-Type': 'application/json',
-            'Content-Disposition': 'attachment; filename="manifest.json"'
-        })
-    respx.get(dl_url3).return_value = manifest
-    # Test Checksum
-    cl = OrdersClient(session, base_url=TEST_URL)
-    await cl.download_order(oid, directory=str(tmpdir), checksum=checksum)
-
-
-@respx.mock
-@pytest.mark.asyncio
-@pytest.mark.parametrize("checksum", [("MD5"), ("SHA256")])
-async def test_checksum_failure(tmpdir,
-                                order_description,
-                                oid,
-                                session,
-                                checksum):
-    # Note: the hashkeys in the mock manifest below were changed
-    # from the correct keys to temporary keys
-    # This should cause the checksum to fail.
-    # Mock an HTTP response for download
-    order_description['state'] = 'success'
-    dl_url1 = TEST_DOWNLOAD_URL + '/asset1'
-    dl_url2 = TEST_DOWNLOAD_URL + '/asset2'
-    dl_url3 = TEST_DOWNLOAD_URL + '/manifest'
-    order_description['_links']['results'] = [{
-        'location': dl_url1
-    }, {
-        'location': dl_url2
-    }, {
-        'location': dl_url3
-    }]
-    get_url = f'{TEST_ORDERS_URL}/{oid}'
-    get_order_response = httpx.Response(HTTPStatus.OK, json=order_description)
-    respx.get(get_url).return_value = get_order_response
-
-    # The first asset in the order:
-    # A 1-byte binary file containing the value "1".
-    asset1_content = b"1"
-    asset1_response = httpx.Response(HTTPStatus.OK,
-                                     content=asset1_content,
-                                     headers={
-                                         'Content-Type':
-                                         'image/tiff',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset1.tif"'
-                                     })
-    respx.get(dl_url1).return_value = asset1_response
-
-    # The second asset in the order is a json file
-    asset2_response = httpx.Response(HTTPStatus.OK,
-                                     json={'foo': 'bar'},
-                                     headers={
-                                         'Content-Type':
-                                         'application/json',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset2.json"'
-                                     })
-    respx.get(dl_url2).return_value = asset2_response
-
-    manifest = httpx.Response(HTTPStatus.OK,
-                              json={
-                                  "name":
-                                  "",
-                                  "files": [{
-                                      "path": "asset1.tif",
-                                      "digests": {
-                                          "md5": "manifest_key1_md5",
-                                          "sha256": "manifest_key1_sha256"
-                                      },
-                                  },
-                                            {
-                                                "path": "asset2.json",
-                                                "digests": {
-                                                    "md5":
-                                                    "manifest_key2_md5",
-                                                    "sha256":
-                                                    "manifest_key2_sha256"
-                                                }
-                                            }]
-                              },
-                              headers={
-                                  'Content-Type':
-                                  'application/json',
-                                  'Content-Disposition':
-                                  'attachment; filename="manifest.json"'
-                              })
-    respx.get(dl_url3).return_value = manifest
-    # Test Checksum
-    cl = OrdersClient(session, base_url=TEST_URL)
-    with pytest.raises(exceptions.ClientError):
-        await cl.download_order(oid, directory=str(tmpdir), checksum=checksum)
-
-
 @respx.mock
 @pytest.mark.asyncio
 async def test_download_asset_img(tmpdir, open_test_img, session):
diff --git a/tests/unit/test_http.py b/tests/unit/test_http.py
index cceb3ea..273f781 100644
--- a/tests/unit/test_http.py
+++ b/tests/unit/test_http.py
@@ -13,7 +13,8 @@
 # the License.
 import logging
 from http import HTTPStatus
-from unittest.mock import Mock
+import math
+from unittest.mock import Mock, patch
 
 import httpx
 import respx
@@ -102,7 +103,8 @@ async def test_session_stream(mock_request):
 
 @respx.mock
 @pytest.mark.asyncio
-async def test_session_request_retry(mock_request, mock_response):
+async def test_session_request_retry(mock_request):
+    """Test the retry in the Session.request method"""
     async with http.Session() as ps:
         route = respx.get(TEST_URL)
         route.side_effect = [
@@ -110,7 +112,9 @@ async def test_session_request_retry(mock_request, mock_response):
             httpx.Response(HTTPStatus.OK, json={})
         ]
 
-        ps.retry_wait_time = 0  # lets not slow down tests for this
+        # let's not actually introduce a wait into the tests
+        ps.max_retry_backoff = 0
+
         resp = await ps.request(mock_request)
         assert resp
         assert route.call_count == 2
@@ -118,15 +122,40 @@ async def test_session_request_retry(mock_request, mock_response):
 
 @respx.mock
 @pytest.mark.asyncio
-async def test_session_retry(mock_request):
-    async with http.Session() as ps:
+async def test_session__retry():
+    """A unit test for the _retry function"""
+
+    async def test_func():
+        # directly trigger the retry logic
+        raise exceptions.TooManyRequests
+
+    with patch('planet.http.Session._calculate_wait') as mock_wait:
+        # let's not actually introduce a wait into the tests
+        mock_wait.return_value = 0
+
+        async with http.Session() as ps:
+            with pytest.raises(exceptions.TooManyRequests):
+                await ps._retry(test_func)
+
+        calls = mock_wait.call_args_list
+        args = [c[0] for c in calls]
+        assert args == [(1, 64), (2, 64), (3, 64), (4, 64), (5, 64)]
+
+
+def test__calculate_wait():
+    max_retry_backoff = 20
+    wait_times = [
+        http.Session._calculate_wait(i + 1, max_retry_backoff)
+        for i in range(5)
+    ]
 
-        async def test_func():
-            raise exceptions.TooManyRequests
+    # (min, max): 2**n to 2**n + 1, last entry hit threshold
+    expected_times = [2, 4, 8, 16, 20]
 
-        ps.retry_wait_time = 0
-        with pytest.raises(exceptions.TooManyRequests):
-            await ps._retry(test_func)
+    for wait, expected in zip(wait_times, expected_times):
+        # this doesn't really test the randomness but does test exponential
+        # and threshold
+        assert math.floor(wait) == expected
 
 
 @respx.mock