From b10af4541cd398fd6c7d0b1b03a637a965d7aaaa Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Wed, 11 Jan 2023 12:19:45 +0300 Subject: [PATCH 1/3] test for current behaviour of retry_operation_impl --- ydb/_errors.py | 52 +++++++++++++++++++++ ydb/table.py | 12 +++++ ydb/table_test.py | 116 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 ydb/_errors.py create mode 100644 ydb/table_test.py diff --git a/ydb/_errors.py b/ydb/_errors.py new file mode 100644 index 00000000..878f4077 --- /dev/null +++ b/ydb/_errors.py @@ -0,0 +1,52 @@ +from dataclasses import dataclass +from typing import Optional + +from ydb import issues + +_errors_retriable_fast_backoff_types = [ + issues.Unavailable, +] +_errors_retriable_slow_backoff_types = [ + issues.Aborted, + issues.BadSession, + issues.Overloaded, + issues.SessionPoolEmpty, + issues.ConnectionError, +] +_errors_retriable_slow_backoff_idempotent_types = [ + issues.Undetermined, +] + + +def check_retriable_error(err, retry_settings, attempt) -> "ErrorRetryInfo": + if isinstance(err, issues.NotFound): + if retry_settings.retry_not_found: + return ErrorRetryInfo(True, retry_settings.fast_backoff.calc_timeout(attempt)) + else: + return ErrorRetryInfo(False, None) + + if isinstance(err, issues.InternalError): + if retry_settings.retry_internal_error: + return ErrorRetryInfo(True, retry_settings.slow_backoff.calc_timeout(attempt)) + else: + return ErrorRetryInfo(False, None) + + for t in _errors_retriable_fast_backoff_types: + if isinstance(err, t): + return ErrorRetryInfo(True, retry_settings.fast_backoff.calc_timeout(attempt)) + + for t in _errors_retriable_slow_backoff_types: + if isinstance(err, t): + return ErrorRetryInfo(True, retry_settings.slow_backoff.calc_timeout(attempt)) + + if retry_settings.idempotent: + for t in _errors_retriable_slow_backoff_idempotent_types: + return ErrorRetryInfo(True, retry_settings.slow_backoff.calc_timeout(attempt)) + + return ErrorRetryInfo(False, None) + + +@dataclass +class ErrorRetryInfo: + is_retriable: bool + sleep_timeout_seconds: Optional[float] diff --git a/ydb/table.py b/ydb/table.py index ebc4ece3..2f4fb388 100644 --- a/ydb/table.py +++ b/ydb/table.py @@ -916,12 +916,24 @@ class YdbRetryOperationSleepOpt(object): def __init__(self, timeout): self.timeout = timeout + def __eq__(self, other): + return type(self) == type(other) and self.timeout == other.timeout + + def __repr__(self): + return "YdbRetryOperationSleepOpt(%s)" % self.timeout + class YdbRetryOperationFinalResult(object): def __init__(self, result): self.result = result self.exc = None + def __eq__(self, other): + return type(self) == type(other) and self.result == other.result and self.exc == other.exc + + def __repr__(self): + return "YdbRetryOperationFinalResult(%s, exc=%s)" % (self.result, self.exc) + def set_exception(self, exc): self.exc = exc diff --git a/ydb/table_test.py b/ydb/table_test.py new file mode 100644 index 00000000..cfbb2c61 --- /dev/null +++ b/ydb/table_test.py @@ -0,0 +1,116 @@ +import random +from unittest import mock +from ydb import retry_operation_impl, YdbRetryOperationFinalResult, issues, YdbRetryOperationSleepOpt, RetrySettings + + +def test_retry_operation_impl(monkeypatch): + monkeypatch.setattr("random.random", lambda: 0.5) + monkeypatch.setattr(issues.Error, "__eq__", + lambda self, other: type(self) == type(other) and self.message == other.message + ) + + retry_once_settings = RetrySettings( + max_retries=1, + on_ydb_error_callback=mock.Mock(), + ) + retry_once_settings.unknown_error_handler = mock.Mock() + + def get_results(callee): + res_generator = retry_operation_impl(callee, retry_settings=retry_once_settings) + results = [] + exc = None + try: + for res in res_generator: + results.append(res) + if isinstance(res, YdbRetryOperationFinalResult): + break + except Exception as e: + exc = e + + return results, exc + + class TestException(Exception): + def __init__(self, message): + super(TestException, self).__init__(message) + self.message = message + + def __eq__(self, other): + return type(self) == type(other) and self.message == other.message + + def check_unretriable_error(err_type, call_ydb_handler): + retry_once_settings.on_ydb_error_callback.reset_mock() + retry_once_settings.unknown_error_handler.reset_mock() + + results = get_results(mock.Mock(side_effect=[err_type("test1"), err_type("test2")])) + yields = results[0] + exc = results[1] + + assert yields == [] + assert exc == err_type("test1") + + if call_ydb_handler: + assert retry_once_settings.on_ydb_error_callback.call_count == 1 + retry_once_settings.on_ydb_error_callback.assert_called_with(err_type("test1")) + + assert retry_once_settings.unknown_error_handler.call_count == 0 + else: + assert retry_once_settings.on_ydb_error_callback.call_count == 0 + + assert retry_once_settings.unknown_error_handler.call_count == 1 + retry_once_settings.unknown_error_handler.assert_called_with(err_type("test1")) + + + def check_retriable_error(err_type, backoff): + retry_once_settings.on_ydb_error_callback.reset_mock() + + results = get_results(mock.Mock(side_effect=[err_type("test1"), err_type("test2")])) + yields = results[0] + exc = results[1] + + if backoff: + assert [YdbRetryOperationSleepOpt(backoff.calc_timeout(0)), YdbRetryOperationSleepOpt(backoff.calc_timeout(1))] == yields + else: + assert [] == yields + + assert exc == err_type("test2") + + assert retry_once_settings.on_ydb_error_callback.call_count == 2 + retry_once_settings.on_ydb_error_callback.assert_any_call(err_type("test1")) + retry_once_settings.on_ydb_error_callback.assert_called_with(err_type("test2")) + + assert retry_once_settings.unknown_error_handler.call_count == 0 + + # check ok + assert get_results(lambda: True) == ([YdbRetryOperationFinalResult(True)], None) + + # check retry error and return result + assert get_results(mock.Mock(side_effect=[issues.Overloaded("test"), True])) == ([ + YdbRetryOperationSleepOpt(retry_once_settings.slow_backoff.calc_timeout(0)), + YdbRetryOperationFinalResult(True), + ], None) + + # check errors + check_retriable_error(issues.Aborted, None) + check_retriable_error(issues.BadSession, None) + + check_retriable_error(issues.NotFound, None) + with mock.patch.object(retry_once_settings, "retry_not_found", False): + check_unretriable_error(issues.NotFound, True) + + check_retriable_error(issues.InternalError, None) + with mock.patch.object(retry_once_settings, "retry_internal_error", False): + check_unretriable_error(issues.InternalError, True) + + check_retriable_error(issues.Overloaded, retry_once_settings.slow_backoff) + check_retriable_error(issues.SessionPoolEmpty, retry_once_settings.slow_backoff) + check_retriable_error(issues.ConnectionError, retry_once_settings.slow_backoff) + + check_retriable_error(issues.Unavailable, retry_once_settings.fast_backoff) + + check_unretriable_error(issues.Undetermined, True) + with mock.patch.object(retry_once_settings, "idempotent", True): + check_retriable_error(issues.Unavailable, retry_once_settings.fast_backoff) + + check_unretriable_error(issues.Error, True) + check_unretriable_error(TestException, False) + From 71ec0db14178348386eb6af81bd5a92b4aee9b91 Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Wed, 11 Jan 2023 14:02:35 +0300 Subject: [PATCH 2/3] extract error check to external function --- ydb/_errors.py | 2 +- ydb/table.py | 61 ++++++++++++++------------------------------------ 2 files changed, 18 insertions(+), 45 deletions(-) diff --git a/ydb/_errors.py b/ydb/_errors.py index 878f4077..002e841b 100644 --- a/ydb/_errors.py +++ b/ydb/_errors.py @@ -18,7 +18,7 @@ ] -def check_retriable_error(err, retry_settings, attempt) -> "ErrorRetryInfo": +def check_retriable_error(err, retry_settings, attempt): if isinstance(err, issues.NotFound): if retry_settings.retry_not_found: return ErrorRetryInfo(True, retry_settings.fast_backoff.calc_timeout(attempt)) diff --git a/ydb/table.py b/ydb/table.py index 2f4fb388..4622078c 100644 --- a/ydb/table.py +++ b/ydb/table.py @@ -21,6 +21,7 @@ _tx_ctx_impl, tracing, ) +from ._errors import check_retriable_error try: from . import interceptor @@ -950,56 +951,28 @@ def retry_operation_impl(callee, retry_settings=None, *args, **kwargs): if result.exc is not None: raise result.exc - except ( - issues.Aborted, - issues.BadSession, - issues.NotFound, - issues.InternalError, - ) as e: - status = e - retry_settings.on_ydb_error_callback(e) - - if isinstance(e, issues.NotFound) and not retry_settings.retry_not_found: - raise e - - if ( - isinstance(e, issues.InternalError) - and not retry_settings.retry_internal_error - ): - raise e - - except ( - issues.Overloaded, - issues.SessionPoolEmpty, - issues.ConnectionError, - ) as e: - status = e - retry_settings.on_ydb_error_callback(e) - yield YdbRetryOperationSleepOpt( - retry_settings.slow_backoff.calc_timeout(attempt) - ) - - except issues.Unavailable as e: + except issues.Error as e: status = e retry_settings.on_ydb_error_callback(e) - yield YdbRetryOperationSleepOpt( - retry_settings.fast_backoff.calc_timeout(attempt) - ) - except issues.Undetermined as e: - status = e - retry_settings.on_ydb_error_callback(e) - if not retry_settings.idempotent: - # operation is not idempotent, so we cannot retry. + retriable_info = check_retriable_error(e, retry_settings, attempt) + if not retriable_info.is_retriable: raise - yield YdbRetryOperationSleepOpt( - retry_settings.fast_backoff.calc_timeout(attempt) - ) + skip_yield_error_types = [ + issues.Aborted, + issues.BadSession, + issues.NotFound, + issues.InternalError, + ] - except issues.Error as e: - retry_settings.on_ydb_error_callback(e) - raise + yield_sleep = True + for t in skip_yield_error_types: + if isinstance(e, t): + yield_sleep = False + + if yield_sleep: + yield YdbRetryOperationSleepOpt(retriable_info.sleep_timeout_seconds) except Exception as e: # you should provide your own handler you want From 164597bcedacdcd7889015187b952d9b38da3a43 Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Wed, 11 Jan 2023 14:20:05 +0300 Subject: [PATCH 3/3] fix by linters --- ydb/_errors.py | 20 ++++++++++++++----- ydb/table.py | 6 +++++- ydb/table_test.py | 51 ++++++++++++++++++++++++++++++++--------------- 3 files changed, 55 insertions(+), 22 deletions(-) diff --git a/ydb/_errors.py b/ydb/_errors.py index 002e841b..e8628586 100644 --- a/ydb/_errors.py +++ b/ydb/_errors.py @@ -21,27 +21,37 @@ def check_retriable_error(err, retry_settings, attempt): if isinstance(err, issues.NotFound): if retry_settings.retry_not_found: - return ErrorRetryInfo(True, retry_settings.fast_backoff.calc_timeout(attempt)) + return ErrorRetryInfo( + True, retry_settings.fast_backoff.calc_timeout(attempt) + ) else: return ErrorRetryInfo(False, None) if isinstance(err, issues.InternalError): if retry_settings.retry_internal_error: - return ErrorRetryInfo(True, retry_settings.slow_backoff.calc_timeout(attempt)) + return ErrorRetryInfo( + True, retry_settings.slow_backoff.calc_timeout(attempt) + ) else: return ErrorRetryInfo(False, None) for t in _errors_retriable_fast_backoff_types: if isinstance(err, t): - return ErrorRetryInfo(True, retry_settings.fast_backoff.calc_timeout(attempt)) + return ErrorRetryInfo( + True, retry_settings.fast_backoff.calc_timeout(attempt) + ) for t in _errors_retriable_slow_backoff_types: if isinstance(err, t): - return ErrorRetryInfo(True, retry_settings.slow_backoff.calc_timeout(attempt)) + return ErrorRetryInfo( + True, retry_settings.slow_backoff.calc_timeout(attempt) + ) if retry_settings.idempotent: for t in _errors_retriable_slow_backoff_idempotent_types: - return ErrorRetryInfo(True, retry_settings.slow_backoff.calc_timeout(attempt)) + return ErrorRetryInfo( + True, retry_settings.slow_backoff.calc_timeout(attempt) + ) return ErrorRetryInfo(False, None) diff --git a/ydb/table.py b/ydb/table.py index 4622078c..6f0a4868 100644 --- a/ydb/table.py +++ b/ydb/table.py @@ -930,7 +930,11 @@ def __init__(self, result): self.exc = None def __eq__(self, other): - return type(self) == type(other) and self.result == other.result and self.exc == other.exc + return ( + type(self) == type(other) + and self.result == other.result + and self.exc == other.exc + ) def __repr__(self): return "YdbRetryOperationFinalResult(%s, exc=%s)" % (self.result, self.exc) diff --git a/ydb/table_test.py b/ydb/table_test.py index cfbb2c61..361719be 100644 --- a/ydb/table_test.py +++ b/ydb/table_test.py @@ -1,13 +1,20 @@ -import random from unittest import mock -from ydb import retry_operation_impl, YdbRetryOperationFinalResult, issues, YdbRetryOperationSleepOpt, RetrySettings +from ydb import ( + retry_operation_impl, + YdbRetryOperationFinalResult, + issues, + YdbRetryOperationSleepOpt, + RetrySettings, +) def test_retry_operation_impl(monkeypatch): monkeypatch.setattr("random.random", lambda: 0.5) - monkeypatch.setattr(issues.Error, "__eq__", - lambda self, other: type(self) == type(other) and self.message == other.message - ) + monkeypatch.setattr( + issues.Error, + "__eq__", + lambda self, other: type(self) == type(other) and self.message == other.message, + ) retry_once_settings = RetrySettings( max_retries=1, @@ -41,7 +48,9 @@ def check_unretriable_error(err_type, call_ydb_handler): retry_once_settings.on_ydb_error_callback.reset_mock() retry_once_settings.unknown_error_handler.reset_mock() - results = get_results(mock.Mock(side_effect=[err_type("test1"), err_type("test2")])) + results = get_results( + mock.Mock(side_effect=[err_type("test1"), err_type("test2")]) + ) yields = results[0] exc = results[1] @@ -50,25 +59,33 @@ def check_unretriable_error(err_type, call_ydb_handler): if call_ydb_handler: assert retry_once_settings.on_ydb_error_callback.call_count == 1 - retry_once_settings.on_ydb_error_callback.assert_called_with(err_type("test1")) + retry_once_settings.on_ydb_error_callback.assert_called_with( + err_type("test1") + ) assert retry_once_settings.unknown_error_handler.call_count == 0 else: assert retry_once_settings.on_ydb_error_callback.call_count == 0 assert retry_once_settings.unknown_error_handler.call_count == 1 - retry_once_settings.unknown_error_handler.assert_called_with(err_type("test1")) - + retry_once_settings.unknown_error_handler.assert_called_with( + err_type("test1") + ) def check_retriable_error(err_type, backoff): retry_once_settings.on_ydb_error_callback.reset_mock() - results = get_results(mock.Mock(side_effect=[err_type("test1"), err_type("test2")])) + results = get_results( + mock.Mock(side_effect=[err_type("test1"), err_type("test2")]) + ) yields = results[0] exc = results[1] if backoff: - assert [YdbRetryOperationSleepOpt(backoff.calc_timeout(0)), YdbRetryOperationSleepOpt(backoff.calc_timeout(1))] == yields + assert [ + YdbRetryOperationSleepOpt(backoff.calc_timeout(0)), + YdbRetryOperationSleepOpt(backoff.calc_timeout(1)), + ] == yields else: assert [] == yields @@ -84,10 +101,13 @@ def check_retriable_error(err_type, backoff): assert get_results(lambda: True) == ([YdbRetryOperationFinalResult(True)], None) # check retry error and return result - assert get_results(mock.Mock(side_effect=[issues.Overloaded("test"), True])) == ([ - YdbRetryOperationSleepOpt(retry_once_settings.slow_backoff.calc_timeout(0)), - YdbRetryOperationFinalResult(True), - ], None) + assert get_results(mock.Mock(side_effect=[issues.Overloaded("test"), True])) == ( + [ + YdbRetryOperationSleepOpt(retry_once_settings.slow_backoff.calc_timeout(0)), + YdbRetryOperationFinalResult(True), + ], + None, + ) # check errors check_retriable_error(issues.Aborted, None) @@ -113,4 +133,3 @@ def check_retriable_error(err_type, backoff): check_unretriable_error(issues.Error, True) check_unretriable_error(TestException, False) -