Repository: Duke-GCB/DukeDSClient

Issue:
Centralize retry constants
I appreciate that these are constants at the top of the file and not buried as magic numbers, but I do think the number of retry constants has expanded enough to warrant a refactoring. As a follow up issue I'd suggest gathering these constants from the various files into a class where they could be grouped/structured/documented more easily.

_Originally posted by @dleehr in https://github.com/Duke-GCB/DukeDSClient/pull/254_

Discussion:

Patch:
diff --git a/ddsc/core/ddsapi.py b/ddsc/core/ddsapi.py
index ded8c79..aace349 100644
--- a/ddsc/core/ddsapi.py
+++ b/ddsc/core/ddsapi.py
@@ -6,19 +6,16 @@ import time
 import datetime
 from ddsc.config import get_user_config_filename
 from ddsc.versioncheck import APP_NAME, get_internal_version_str
+from ddsc.core.retry import RetrySettings
 
 AUTH_TOKEN_CLOCK_SKEW_MAX = 5 * 60  # 5 minutes
 SETUP_GUIDE_URL = "https://github.com/Duke-GCB/DukeDSClient/blob/master/docs/GettingAgentAndUserKeys.md"
-RESOURCE_NOT_CONSISTENT_RETRY_SECONDS = 2
-SERVICE_DOWN_RETRY_SECONDS = 60  # 1 minute
 SERVICE_DOWN_MESSAGE = """Duke Data Service is currently unavailable as, so this operation cannot complete right now. ({})
 The operation will be retried automatically, so no action is required. It will complete once Duke Data Service is available.
 
 To cancel this operation, press Ctrl+C.
 """
-CONNECTION_RETRY_TIMES = 5
 CONNECTION_RETRY_MESSAGE = "Connection failed. Retrying."
-CONNECTION_RETRY_SECONDS = 1
 
 
 def get_user_agent_str():
@@ -31,8 +28,9 @@ def get_user_agent_str():
 
 def retry_when_service_unavailable(func):
     """
-    Decorator that will retry a function while it fails with status code 503 and up to CONNECTION_RETRY_TIMES connection
-    errors. Assumes the first argument to the function will be an object with a set_status_message method.
+    Decorator that will retry a function while it fails with status code 503 and up to
+    RetrySettings.CONNECTION_RETRY_TIMES connection errors. Assumes the first argument to the function will be an
+    object with a set_status_message method.
     :param func: function: will be called until it doesn't fail with DataServiceError status 503, or too many ConnectionErrors
     :return: value returned by func
     """
@@ -50,16 +48,16 @@ def retry_when_service_unavailable(func):
                 return result
             except requests.exceptions.ConnectionError:
                 connection_retries += 1
-                if connection_retries <= CONNECTION_RETRY_TIMES:
+                if connection_retries <= RetrySettings.CONNECTION_RETRY_TIMES:
                     # continue to retry and show message
                     status_msg = CONNECTION_RETRY_MESSAGE
-                    sleep_seconds = CONNECTION_RETRY_SECONDS
+                    sleep_seconds = RetrySettings.CONNECTION_RETRY_SECONDS
                 else:
                     raise
             except DataServiceError as dse:
                 if dse.status_code == 503:
                     status_msg = SERVICE_DOWN_MESSAGE.format(datetime.datetime.utcnow())
-                    sleep_seconds = SERVICE_DOWN_RETRY_SECONDS
+                    sleep_seconds = RetrySettings.SERVICE_DOWN_RETRY_SECONDS
                 else:
                     raise
             time.sleep(sleep_seconds)
@@ -1216,4 +1214,4 @@ def retry_until_resource_is_consistent(func, monitor):
             if not waiting and monitor:
                 monitor.start_waiting()
                 waiting = True
-            time.sleep(RESOURCE_NOT_CONSISTENT_RETRY_SECONDS)
+            time.sleep(RetrySettings.RESOURCE_NOT_CONSISTENT_RETRY_SECONDS)
diff --git a/ddsc/core/download.py b/ddsc/core/download.py
index abeac2f..dab4306 100644
--- a/ddsc/core/download.py
+++ b/ddsc/core/download.py
@@ -8,10 +8,8 @@ from ddsc.core.util import ProgressPrinter
 from ddsc.core.parallel import TaskExecutor, TaskRunner
 from ddsc.core.ddsapi import DataServiceAuth, DataServiceApi
 from ddsc.core.remotestore import RemoteStore, ProjectFile, RemoteFileUrl
+from ddsc.core.retry import RetrySettings
 
-FETCH_EXTERNAL_PUT_RETRY_TIMES = 5
-FETCH_EXTERNAL_RETRY_SECONDS = 20
-RESOURCE_NOT_CONSISTENT_RETRY_SECONDS = 2
 SWIFT_EXPIRED_STATUS_CODE = 401
 S3_EXPIRED_STATUS_CODE = 403
 MISMATCHED_FILE_HASH_WARNING = """
@@ -408,7 +406,7 @@ class RetryChunkDownloader(object):
         self.seek_amt = seek_amt
         self.bytes_to_read = bytes_to_read
         self.retry_times = 0
-        self.max_retry_times = FETCH_EXTERNAL_PUT_RETRY_TIMES
+        self.max_retry_times = RetrySettings.FETCH_EXTERNAL_PUT_RETRY_TIMES
         self.download_context = download_context
         self.actual_bytes_read = 0
         self.remote_store = download_context.create_remote_store()
diff --git a/ddsc/core/fileuploader.py b/ddsc/core/fileuploader.py
index 52af056..ead4041 100644
--- a/ddsc/core/fileuploader.py
+++ b/ddsc/core/fileuploader.py
@@ -3,21 +3,17 @@ Objects to upload a number of chunks from a file to a remote store as part of an
 """
 from __future__ import print_function
 import math
-import time
 import requests
 from multiprocessing import Process, Queue
 from ddsc.core.ddsapi import DataServiceAuth, DataServiceApi, retry_until_resource_is_consistent
 from ddsc.core.util import ProgressQueue, wait_for_processes
 from ddsc.core.localstore import HashData
+from ddsc.core.retry import RetrySettings
 import traceback
 import sys
+import time
 from tenacity import retry, retry_if_exception_type, stop_after_attempt
 
-SEND_EXTERNAL_PUT_RETRY_TIMES = 5
-SEND_EXTERNAL_RETRY_SECONDS = 20
-SEND_EXTERNAL_FORBIDDEN_RETRY_TIMES = 2
-RESOURCE_NOT_CONSISTENT_RETRY_SECONDS = 2
-
 
 class ForbiddenSendExternalException(Exception):
     pass
@@ -206,7 +202,7 @@ class FileUploadOperations(object):
         count = 0
         retry_times = 1
         if http_verb == 'PUT':
-            retry_times = SEND_EXTERNAL_PUT_RETRY_TIMES
+            retry_times = RetrySettings.SEND_EXTERNAL_PUT_RETRY_TIMES
         while True:
             try:
                 return self.data_service.send_external(http_verb, host, url, http_headers, chunk)
@@ -215,7 +211,7 @@ class FileUploadOperations(object):
                 if count < retry_times:
                     if count == 1:  # Only show a warning the first time we fail to send a chunk
                         self._show_retry_warning(host)
-                    time.sleep(SEND_EXTERNAL_RETRY_SECONDS)
+                    time.sleep(RetrySettings.SEND_EXTERNAL_RETRY_SECONDS)
                     self.data_service.recreate_requests_session()
                 else:
                     raise
@@ -391,7 +387,7 @@ class ChunkSender(object):
                 sent_chunks += 1
 
     @retry(retry=retry_if_exception_type(ForbiddenSendExternalException),
-           stop=stop_after_attempt(SEND_EXTERNAL_FORBIDDEN_RETRY_TIMES),
+           stop=stop_after_attempt(RetrySettings.SEND_EXTERNAL_FORBIDDEN_RETRY_TIMES),
            reraise=True)
     def _send_chunk(self, chunk, chunk_num):
         """
diff --git a/ddsc/core/retry.py b/ddsc/core/retry.py
new file mode 100644
index 0000000..66240ec
--- /dev/null
+++ b/ddsc/core/retry.py
@@ -0,0 +1,20 @@
+
+class RetrySettings(object):
+    # DukeDS API Retry Settings
+    # Settings for retrying a ConnectionError connecting to the DukeDS API
+    CONNECTION_RETRY_TIMES = 5
+    CONNECTION_RETRY_SECONDS = 1
+    # Settings for how long to sleep when we receive 503 from DukeDS API (Weekly system maintenance typically)
+    SERVICE_DOWN_RETRY_SECONDS = 60
+    # How long to sleep when waiting for DukeDS API to be consistent (waits forever)
+    RESOURCE_NOT_CONSISTENT_RETRY_SECONDS = 2
+
+    # Backend Store Retry Settings
+    # Settings for retrying when a ConnectionError occurs uploading a file chunk
+    SEND_EXTERNAL_PUT_RETRY_TIMES = 4
+    SEND_EXTERNAL_RETRY_SECONDS = 20
+    # Times to retry after receiving a 403 uploading a file chunk (we recreate the URL before retrying)
+    SEND_EXTERNAL_FORBIDDEN_RETRY_TIMES = 2
+    # Settings for retrying when downloading part of a file
+    FETCH_EXTERNAL_PUT_RETRY_TIMES = 5
+    FETCH_EXTERNAL_RETRY_SECONDS = 20


Test patch:
diff --git a/ddsc/core/tests/test_ddsapi.py b/ddsc/core/tests/test_ddsapi.py
index a924dbf..fc18407 100644
--- a/ddsc/core/tests/test_ddsapi.py
+++ b/ddsc/core/tests/test_ddsapi.py
@@ -6,7 +6,7 @@ from ddsc.core.ddsapi import MultiJSONResponse, DataServiceApi, DataServiceAuth,
 from ddsc.core.ddsapi import MissingInitialSetupError, SoftwareAgentNotFoundError, AuthTokenCreationError, \
     UnexpectedPagingReceivedError, DataServiceError, DSResourceNotConsistentError, \
     retry_until_resource_is_consistent, retry_when_service_unavailable, CONNECTION_RETRY_MESSAGE, \
-    CONNECTION_RETRY_SECONDS, SERVICE_DOWN_RETRY_SECONDS, CONNECTION_RETRY_TIMES
+    RetrySettings
 from mock import MagicMock, Mock, patch, ANY
 
 
@@ -1005,7 +1005,7 @@ class TestRetryWhenServiceUnavailable(TestCase):
         self.raise_error_once = DataServiceError(mock_response, '', '')
         self.assertEqual('result123', self.func('123'))
         self.assertEqual(1, mock_time.sleep.call_count)
-        mock_time.sleep.assert_called_with(SERVICE_DOWN_RETRY_SECONDS)
+        mock_time.sleep.assert_called_with(RetrySettings.SERVICE_DOWN_RETRY_SECONDS)
         self.assertEqual(2, len(self.status_messages))
         self.assertIn('Duke Data Service is currently unavailable', self.status_messages[0])
         self.assertEqual('', self.status_messages[1])
@@ -1026,8 +1026,8 @@ class TestRetryWhenServiceUnavailable(TestCase):
             self.assertEqual('result123', self.func('123'))
         except requests.exceptions.ConnectionError:
             pass
-        self.assertEqual(CONNECTION_RETRY_TIMES, mock_time.sleep.call_count)
-        mock_time.sleep.assert_called_with(CONNECTION_RETRY_SECONDS)
+        self.assertEqual(RetrySettings.CONNECTION_RETRY_TIMES, mock_time.sleep.call_count)
+        mock_time.sleep.assert_called_with(RetrySettings.CONNECTION_RETRY_SECONDS)
         self.assertEqual(1, len(self.status_messages))
         self.assertEqual(CONNECTION_RETRY_MESSAGE, self.status_messages[0])
 
diff --git a/ddsc/core/tests/test_fileuploader.py b/ddsc/core/tests/test_fileuploader.py
index b8817bf..f75da01 100644
--- a/ddsc/core/tests/test_fileuploader.py
+++ b/ddsc/core/tests/test_fileuploader.py
@@ -1,6 +1,6 @@
 from unittest import TestCase
 from ddsc.core.fileuploader import ParallelChunkProcessor, upload_async, FileUploadOperations, \
-    RESOURCE_NOT_CONSISTENT_RETRY_SECONDS, ForbiddenSendExternalException, ChunkSender
+    RetrySettings, ForbiddenSendExternalException, ChunkSender
 from ddsc.core.ddsapi import DSResourceNotConsistentError, DataServiceError
 import requests
 from mock import MagicMock, Mock, patch, call
@@ -72,6 +72,7 @@ class TestFileUploadOperations(TestCase):
         data_service = MagicMock()
         data_service.send_external.side_effect = [Mock(status_code=201)]
         fop = FileUploadOperations(data_service, MagicMock())
+        fop._show_retry_warning = Mock()
         url_json = {
             'http_verb': 'PUT',
             'host': 'something.com',
@@ -80,9 +81,10 @@ class TestFileUploadOperations(TestCase):
         }
         fop.send_file_external(url_json, chunk='DATADATADATA')
         self.assertEqual(1, data_service.send_external.call_count)
+        self.assertEqual(0, fop._show_retry_warning.call_count)
 
-    @patch('ddsc.core.fileuploader.time')
-    def test_send_file_external_retry_put(self, mock_time):
+    @patch('ddsc.core.fileuploader.time.sleep')
+    def test_send_file_external_retry_put(self, mock_sleep):
         data_service = MagicMock()
         data_service.send_external.side_effect = [requests.exceptions.ConnectionError, Mock(status_code=201)]
         fop = FileUploadOperations(data_service, MagicMock())
@@ -95,8 +97,8 @@ class TestFileUploadOperations(TestCase):
         fop.send_file_external(url_json, chunk='DATADATADATA')
         self.assertEqual(2, data_service.send_external.call_count)
 
-    @patch('ddsc.core.fileuploader.time')
-    def test_send_file_external_403_exception(self, mock_time):
+    @patch('ddsc.core.fileuploader.time.sleep')
+    def test_send_file_external_403_exception(self, mock_sleep):
         data_service = MagicMock()
         data_service.send_external.side_effect = [Mock(status_code=403)]
         fop = FileUploadOperations(data_service, MagicMock())
@@ -111,8 +113,8 @@ class TestFileUploadOperations(TestCase):
         self.assertEqual(str(raised_exception.exception),
                          'Failed to send file to external store. Error:403 something.com/putdata')
 
-    @patch('ddsc.core.fileuploader.time')
-    def test_send_file_external_retry_put_fail_after_5_times(self, mock_time):
+    @patch('ddsc.core.fileuploader.time.sleep')
+    def test_send_file_external_retry_put_fail_after_4_retries(self, mock_sleep):
         data_service = MagicMock()
         connection_err = requests.exceptions.ConnectionError
         data_service.send_external.side_effect = [connection_err, connection_err, connection_err, connection_err,
@@ -124,13 +126,15 @@ class TestFileUploadOperations(TestCase):
             'url': '/putdata',
             'http_headers': [],
         }
+        fop._show_retry_warning = Mock()
         with self.assertRaises(requests.exceptions.ConnectionError):
             fop.send_file_external(url_json, chunk='DATADATADATA')
-        self.assertEqual(5, data_service.send_external.call_count)
-        self.assertEqual(4, data_service.recreate_requests_session.call_count)
+        self.assertEqual(4, data_service.send_external.call_count)
+        self.assertEqual(3, data_service.recreate_requests_session.call_count)
+        self.assertEqual(1, fop._show_retry_warning.call_count)
 
-    @patch('ddsc.core.fileuploader.time')
-    def test_send_file_external_succeeds_3rd_time(self, mock_time):
+    @patch('ddsc.core.fileuploader.time.sleep')
+    def test_send_file_external_succeeds_3rd_time(self, mock_sleep):
         data_service = MagicMock()
         connection_err = requests.exceptions.ConnectionError
         data_service.send_external.side_effect = [connection_err, connection_err, Mock(status_code=201)]
@@ -145,7 +149,8 @@ class TestFileUploadOperations(TestCase):
         self.assertEqual(3, data_service.send_external.call_count)
         self.assertEqual(2, data_service.recreate_requests_session.call_count)
 
-    def test_send_file_external_no_retry_post(self):
+    @patch('ddsc.core.fileuploader.time.sleep')
+    def test_send_file_external_no_retry_post(self, mock_sleep):
         data_service = MagicMock()
         data_service.send_external.side_effect = [requests.exceptions.ConnectionError]
         fop = FileUploadOperations(data_service, MagicMock())
@@ -168,7 +173,7 @@ class TestFileUploadOperations(TestCase):
                           remote_file_id="456")
         data_service.complete_upload.assert_called()
 
-    @patch('ddsc.core.fileuploader.time.sleep')
+    @patch('ddsc.core.ddsapi.time.sleep')
     def test_create_upload_with_one_pause(self, mock_sleep):
         data_service = MagicMock()
         response = Mock()
@@ -182,9 +187,9 @@ class TestFileUploadOperations(TestCase):
         path_data.name.return_value = '/tmp/data.dat'
         upload_id = fop.create_upload(project_id='12', path_data=path_data, hash_data=MagicMock())
         self.assertEqual(upload_id, '123')
-        mock_sleep.assert_called_with(RESOURCE_NOT_CONSISTENT_RETRY_SECONDS)
+        mock_sleep.assert_called_with(RetrySettings.RESOURCE_NOT_CONSISTENT_RETRY_SECONDS)
 
-    @patch('ddsc.core.fileuploader.time.sleep')
+    @patch('ddsc.core.ddsapi.time.sleep')
     def test_create_upload_with_two_pauses(self, mock_sleep):
         data_service = MagicMock()
         response = Mock()
@@ -200,10 +205,10 @@ class TestFileUploadOperations(TestCase):
         upload_id = fop.create_upload(project_id='12', path_data=path_data, hash_data=MagicMock())
         self.assertEqual(upload_id, '124')
         mock_sleep.assert_has_calls([
-            call(RESOURCE_NOT_CONSISTENT_RETRY_SECONDS),
-            call(RESOURCE_NOT_CONSISTENT_RETRY_SECONDS)])
+            call(RetrySettings.RESOURCE_NOT_CONSISTENT_RETRY_SECONDS),
+            call(RetrySettings.RESOURCE_NOT_CONSISTENT_RETRY_SECONDS)])
 
-    @patch('ddsc.core.fileuploader.time.sleep')
+    @patch('ddsc.core.ddsapi.time.sleep')
     def test_create_upload_with_one_pause_then_failure(self, mock_sleep):
         data_service = MagicMock()
         response = Mock()