-
Notifications
You must be signed in to change notification settings - Fork 1.2k
PYTHON-5885 Client Backpressure with baseBackoffMS #2877
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
base: main
Are you sure you want to change the base?
Changes from all commits
1464c48
733b906
f82d99d
18c3c2e
6486561
bc648de
576e870
ac17fae
aa7e1d4
49cb18d
68268f9
acd7d89
8c06ebc
2cf06dc
31edb0e
f9316a7
4486c68
4d4eec6
dee8467
d4300ca
eacbeca
f512029
a6e3b3c
a553b0b
f3ef3d4
5a675b2
bf4c0d5
b7c1475
3a22ee2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2794,6 +2794,7 @@ def __init__( | |
| self._attempt_number = 0 | ||
| self._is_run_command = is_run_command | ||
| self._is_aggregate_write = is_aggregate_write | ||
| self._base_backoff_ms: Optional[float] = None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, |
||
|
|
||
| async def run(self) -> T: | ||
| """Runs the supplied func() and attempts a retry | ||
|
|
@@ -2843,26 +2844,29 @@ async def run(self) -> T: | |
|
|
||
| # Execute specialized catch on read | ||
| if self._is_read: | ||
| if isinstance(exc, (ConnectionFailure, OperationFailure)): | ||
| if isinstance(exc_to_check, (ConnectionFailure, OperationFailure)): | ||
| # ConnectionFailures do not supply a code property | ||
| exc_code = getattr(exc, "code", None) | ||
| overloaded = exc.has_error_label("SystemOverloadedError") | ||
| exc_code = getattr(exc_to_check, "code", None) | ||
| overloaded = exc_to_check.has_error_label("SystemOverloadedError") | ||
| if overloaded: | ||
| self._max_retries = self._client.options.max_adaptive_retries | ||
| always_retryable = exc.has_error_label("RetryableError") and overloaded | ||
| self._base_backoff_ms = getattr(exc_to_check, "_base_backoff_ms", None) | ||
| always_retryable = ( | ||
| exc_to_check.has_error_label("RetryableError") and overloaded | ||
| ) | ||
| if not self._client.options.retry_reads or ( | ||
| not always_retryable | ||
| and ( | ||
| self._is_not_eligible_for_retry() | ||
| or ( | ||
| isinstance(exc, OperationFailure) | ||
| isinstance(exc_to_check, OperationFailure) | ||
| and exc_code not in helpers_shared._RETRYABLE_ERROR_CODES | ||
| ) | ||
| ) | ||
| ): | ||
| raise | ||
| self._retrying = True | ||
| self._last_error = exc | ||
| self._last_error = exc_to_check | ||
| self._attempt_number += 1 | ||
|
|
||
| # Revert back to starting state only if the first | ||
|
|
@@ -2889,6 +2893,7 @@ async def run(self) -> T: | |
| overloaded = exc_to_check.has_error_label("SystemOverloadedError") | ||
| if overloaded: | ||
| self._max_retries = self._client.options.max_adaptive_retries | ||
| self._base_backoff_ms = getattr(exc_to_check, "_base_backoff_ms", None) | ||
| always_retryable = exc_to_check.has_error_label("RetryableError") and overloaded | ||
|
|
||
| # Always retry abortTransaction and commitTransaction up to once | ||
|
|
@@ -2932,7 +2937,10 @@ async def run(self) -> T: | |
|
|
||
| self._always_retryable = always_retryable | ||
| if overloaded: | ||
| delay = self._retry_policy.backoff(self._attempt_number) | ||
| delay = self._retry_policy.backoff( | ||
| self._attempt_number, | ||
| self._base_backoff_ms / 1000 if self._base_backoff_ms else None, | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ye Olde
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The server should never send |
||
| if not await self._retry_policy.should_retry(self._attempt_number, delay): | ||
| if exc_to_check.has_error_label("NoWritesPerformed") and self._last_error: | ||
| raise self._last_error from exc | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,9 +223,9 @@ async def test_01_operation_retry_uses_exponential_backoff(self, random_func): | |
| end1 = perf_counter() | ||
|
|
||
| # f. Compare the times between the two runs. | ||
| # The sum of 2 backoffs is 0.3 seconds. There is a 0.3-second window to account for potential variance between the two | ||
| # The sum of 2 backoffs is 0.6 seconds. There is a 0.6-second window to account for potential variance between the two | ||
| # runs. | ||
| self.assertTrue(abs((end1 - start1) - (end0 - start0 + 0.3)) < 0.3) | ||
| self.assertTrue(abs((end1 - start1) - (end0 - start0 + 0.6)) < 0.6) | ||
|
|
||
| @async_client_context.require_failCommand_appName | ||
| async def test_03_overload_retries_limited(self): | ||
|
|
@@ -294,6 +294,70 @@ async def test_04_overload_retries_limited_configured(self): | |
| # 6. Assert that the total number of started commands is max_retries + 1. | ||
| self.assertEqual(len(self.listener.started_events), max_retries + 1) | ||
|
|
||
| @unittest.skipIf( | ||
| sys.platform == "darwin", | ||
| "externalClientBaseBackoffMS is not supported on macOS", | ||
| ) | ||
| @patch("random.random") | ||
| @async_client_context.require_version_min(9, 0, 0, -1) | ||
| @async_client_context.require_failCommand_appName | ||
| async def test_05_overload_errors_with_basebackoffms_override_backoff(self, random_func): | ||
| # Drivers should test that overload errors with `baseBackoffMS` override the default backoff duration. | ||
|
|
||
| # 1. Let `client` be a `MongoClient`. | ||
| client = self.client | ||
|
|
||
| # 2. Let `coll` be a collection. | ||
| coll = client.test.test | ||
|
|
||
| # 3. Configure the random number generator used for exponential backoff jitter to always return a number as | ||
| # close as possible to `1`. | ||
| random_func.return_value = 1 | ||
|
|
||
| # 4. Configure the following failPoint: | ||
| fail_point = dict( | ||
| mode="alwaysOn", | ||
| data=dict( | ||
| failCommands=["insert"], | ||
| errorCode=462, | ||
| errorLabels=["SystemOverloadedError", "RetryableError"], | ||
| appName=self.app_name, | ||
| ), | ||
| ) | ||
| async with self.fail_point(fail_point): | ||
| # 5. Insert the document `{ a: 1 }`. Expect that the command errors. Measure the duration of the command | ||
| # execution. | ||
| start0 = perf_counter() | ||
| with self.assertRaises(OperationFailure): | ||
| await coll.insert_one({"a": 1}) | ||
| end0 = perf_counter() | ||
| exponential_backoff_time = end0 - start0 | ||
|
|
||
| # 6. Run the following command to set up `baseBackoffMS` on overload errors. | ||
| try: | ||
| await client.admin.command("setParameter", 1, externalClientBaseBackoffMS=50) | ||
|
|
||
| # 7. Execute step 5 again. | ||
| start1 = perf_counter() | ||
| with self.assertRaises(OperationFailure) as ctx: | ||
| await coll.insert_one({"a": 1}) | ||
| end1 = perf_counter() | ||
| with_base_backoff_ms_time = end1 - start1 | ||
|
|
||
| # 8. Assert the server attached `baseBackoffMS` to the error and the driver parsed it. | ||
| self.assertEqual(ctx.exception._base_backoff_ms, 50) | ||
| finally: | ||
| # 9. Run the following command to disable `baseBackoffMS` on overload errors. | ||
| await client.admin.command("setParameter", 1, externalClientBaseBackoffMS=0) | ||
|
|
||
| # 10. Assert absolute bounds on each run's duration. | ||
| # A run can never be faster than the sum of its backoffs. | ||
| # With jitter pinned to 1, the default backoffs are 0.2 + 0.4 = 0.6s | ||
| # and the baseBackoffMS=50 backoffs are 0.1 + 0.2 = 0.3s. | ||
| self.assertGreaterEqual(exponential_backoff_time, 0.6) | ||
| self.assertGreaterEqual(with_base_backoff_ms_time, 0.3) | ||
| self.assertLess(with_base_backoff_ms_time, 0.6) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we care if unrelated perf issues push this past 0.6?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perf issues are an annoying thorn in our side for this type of test: it's more resilient to assert
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we have a way to deal with flaky tests (I think?) so maybe this just goes into that category, unless we do something crazy like < |
||
|
|
||
|
|
||
| # Location of JSON test specifications. | ||
| if _IS_SYNC: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the actual backoff duration and how does changing from
attempt -1toattempthelp to implement ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attempt - 1 -> attemptis to comply with the latest spec, which conforms to the server's exponential backoff formula for consistency. The actual backoff duration for two differentbackoffcalls with the sameattemptargument can differ due tobase_backoff.