-
Notifications
You must be signed in to change notification settings - Fork 1.5k
JAVA-5950 Update Transactions Convenient API with exponential backoff on retries #1899
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: backpressure
Are you sure you want to change the base?
Changes from all commits
7b27940
1094102
024e420
8071de6
0f6e118
19f296f
455f352
85ec5ef
5f2157a
f94d9c7
8c322c6
14f339d
f593950
d12e1f3
26da297
a1d5bca
e8857e0
b8d7abf
c2a91bf
8957883
e008a43
a54d95a
553d6ba
c0136e1
09a1291
520fead
478e242
365f054
098b4b4
4df0f94
9f34468
88071e0
e59c4cb
9a925e1
4bbef70
1c97dc7
e32f90d
0b3ee81
fda498c
e5ae458
24d4ee5
4756404
f22198e
9dcd5c0
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 |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
| * Copyright 2008-present MongoDB, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.mongodb.internal.time; | ||
|
|
||
| import com.mongodb.internal.VisibleForTesting; | ||
|
|
||
| import java.util.concurrent.ThreadLocalRandom; | ||
| import java.util.function.DoubleSupplier; | ||
|
|
||
| import static com.mongodb.assertions.Assertions.assertTrue; | ||
| import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE; | ||
|
|
||
| /** | ||
| * Provides exponential backoff calculations with jitter for retry scenarios. | ||
| */ | ||
| public final class ExponentialBackoff { | ||
|
|
||
| // Constants for transaction retry backoff | ||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| static final double TRANSACTION_BASE_MS = 5.0; | ||
|
Member
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.
|
||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| static final double TRANSACTION_MAX_MS = 500.0; | ||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| static final double TRANSACTION_GROWTH = 1.5; | ||
|
|
||
| // TODO-JAVA-6079 | ||
| private static DoubleSupplier testJitterSupplier = null; | ||
|
|
||
| private ExponentialBackoff() { | ||
| } | ||
|
|
||
| /** | ||
| * Calculate the backoff in milliseconds for transaction retries. | ||
| * | ||
| * @param attemptNumber 0-based attempt number | ||
| * @return The calculated backoff in milliseconds. | ||
| */ | ||
| public static long calculateTransactionBackoffMs(final int attemptNumber) { | ||
| assertTrue(attemptNumber > 0, "Attempt number must be greater than 0 in the context of transaction backoff calculation"); | ||
| double jitter = testJitterSupplier != null | ||
| ? testJitterSupplier.getAsDouble() | ||
| : ThreadLocalRandom.current().nextDouble(); | ||
| return Math.round(jitter * Math.min( | ||
| TRANSACTION_BASE_MS * Math.pow(TRANSACTION_GROWTH, attemptNumber - 1), | ||
| TRANSACTION_MAX_MS)); | ||
| } | ||
|
|
||
| /** | ||
| * Set a custom jitter supplier for testing purposes. | ||
| * | ||
| * @param supplier A DoubleSupplier that returns values in [0, 1) range. | ||
|
Member
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. We should include 1 in the interval: [0, 1], because tests specify 1. Ideally, instead of tests specifying the jitter in this PR, they should have disabled/enabled jitter: it is not conceivable that we will ever need any other behavior here than an switch (and in the highly unlikely situation that we need something else, we can always support that later). The only reason I am not pushing for a switch now is because we hopefully will introduce a switch in https://jira.mongodb.org/browse/JAVA-6079, and remove the temporary methods The same goes about the spec. If the spec was formulated in terms of a switch, neither this Slack discussion nor this https://jira.mongodb.org/browse/DRIVERS-3370 would need to exist. P.S. Note that the specification not having been formulated in terms of a switch does not prevent us from implementing tests using a switch for jitter, because both approaches are equivalent for the current tests. |
||
| */ | ||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| public static void setTestJitterSupplier(final DoubleSupplier supplier) { | ||
| testJitterSupplier = supplier; | ||
| } | ||
|
|
||
| /** | ||
| * Clear the test jitter supplier, reverting to default ThreadLocalRandom behavior. | ||
| */ | ||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| public static void clearTestJitterSupplier() { | ||
| testJitterSupplier = null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /* | ||
| * Copyright 2008-present MongoDB, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.mongodb.internal.time; | ||
|
|
||
| /** | ||
| * Avoid using this class directly and prefer using other program elements from {@link com.mongodb.internal.time}, if possible. | ||
| * <p> | ||
| * We do not use {@link System#nanoTime()} directly in the rest of the {@link com.mongodb.internal.time} package, | ||
| * and use {@link SystemNanoTime#get()} instead because we need to tamper with it via {@code Mockito.mockStatic}, | ||
| * and mocking methods of {@link System} class is both impossible and unwise. | ||
| */ | ||
| public final class SystemNanoTime { | ||
| private SystemNanoTime() { | ||
| } | ||
|
|
||
| public static long get() { | ||
| return System.nanoTime(); | ||
| } | ||
| } |
|
Member
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. I left multiple suggestions about removing comments in this file. All of those comments have the following in common - they explain straightforward code and repeat the assertions / assertion messages in the code. Were they generated with an AI tool? |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,81 @@ | ||||||||||||
| /* | ||||||||||||
| * Copyright 2008-present MongoDB, Inc. | ||||||||||||
| * | ||||||||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||
| * you may not use this file except in compliance with the License. | ||||||||||||
| * You may obtain a copy of the License at | ||||||||||||
| * | ||||||||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||
| * | ||||||||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||
| * See the License for the specific language governing permissions and | ||||||||||||
| * limitations under the License. | ||||||||||||
| */ | ||||||||||||
|
|
||||||||||||
| package com.mongodb.internal.time; | ||||||||||||
|
|
||||||||||||
| import org.junit.jupiter.api.Test; | ||||||||||||
|
|
||||||||||||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||||||||||||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||||||||||||
|
|
||||||||||||
| public class ExponentialBackoffTest { | ||||||||||||
|
Member
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. Original suggestion #1852 (comment) was auto-resolved, but the corresponding change 1655dad hasn't been included in the current PR:
Suggested change
|
||||||||||||
| // Expected backoffs with jitter=1.0 and growth factor ExponentialBackoff.TRANSACTION_GROWTH | ||||||||||||
|
Member
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.
Suggested change
|
||||||||||||
| private static final double[] EXPECTED_BACKOFFS_MAX_VALUES = {5.0, 7.5, 11.25, 16.875, 25.3125, 37.96875, 56.953125, 85.4296875, 128.14453125, | ||||||||||||
| 192.21679688, 288.32519531, 432.48779297, 500.0}; | ||||||||||||
|
|
||||||||||||
| @Test | ||||||||||||
| void testCalculateTransactionBackoffMs() { | ||||||||||||
| // Test that the backoff sequence follows the expected pattern with growth factor ExponentialBackoff.TRANSACTION_GROWTH | ||||||||||||
| // Expected sequence (without jitter): 5, 7.5, 11.25, ... | ||||||||||||
| // With jitter, actual values will be between 0 and these maxima | ||||||||||||
|
|
||||||||||||
| for (int attemptNumber = 1; attemptNumber <= EXPECTED_BACKOFFS_MAX_VALUES.length; attemptNumber++) { | ||||||||||||
| long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber); | ||||||||||||
| long expectedBackoff = Math.round(EXPECTED_BACKOFFS_MAX_VALUES[attemptNumber - 1]); | ||||||||||||
| assertTrue(backoff >= 0 && backoff <= expectedBackoff, | ||||||||||||
| String.format("Attempt %d: backoff should be 0-%d ms, got: %d", attemptNumber, | ||||||||||||
|
Comment on lines
+30
to
+39
Member
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 comment at the top of the test method body does not seem to be useful: it explains the short and straightforward code of the test. Comments, like any other code, are not free: they have to be reviewed, maintained, read by code readers. Comments should not be used to explain what a trivial piece of code does. Let's remove the comment. |
||||||||||||
| expectedBackoff, backoff)); | ||||||||||||
|
Comment on lines
+39
to
+40
Member
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. Original suggestion #1852 (comment) was auto-resolved, but the corresponding change f5274cb hasn't been fully included in the current PR:
Suggested change
|
||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Test | ||||||||||||
| void testCalculateTransactionBackoffMsRespectsMaximum() { | ||||||||||||
| // Even at high attempt numbers, backoff should never exceed ExponentialBackoff.TRANSACTION_MAX_MS | ||||||||||||
| for (int attemptNumber = 1; attemptNumber < 26; attemptNumber++) { | ||||||||||||
|
Member
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. Let's make the number of iterations here based on
Suggested change
|
||||||||||||
| long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber); | ||||||||||||
| assertTrue(backoff >= 0 && backoff <= ExponentialBackoff.TRANSACTION_MAX_MS, | ||||||||||||
| String.format("Attempt %d: backoff should be capped at %f ms, got: %d ms", | ||||||||||||
|
Comment on lines
+45
to
+50
Member
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 "Even at high attempt numbers, backoff should never exceed ExponentialBackoff.TRANSACTION_MAX_MS" comment adds no information, as it repeats both the straightforward assertion in the code, as well as the assertion message. Let's remove the comment. |
||||||||||||
| attemptNumber, ExponentialBackoff.TRANSACTION_MAX_MS, backoff)); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Test | ||||||||||||
| void testCustomJitter() { | ||||||||||||
| // Test with jitter = 1.0 | ||||||||||||
| ExponentialBackoff.setTestJitterSupplier(() -> 1.0); | ||||||||||||
| try { | ||||||||||||
| for (int attemptNumber = 1; attemptNumber <= EXPECTED_BACKOFFS_MAX_VALUES.length; attemptNumber++) { | ||||||||||||
| long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber); | ||||||||||||
| long expected = Math.round(EXPECTED_BACKOFFS_MAX_VALUES[attemptNumber - 1]); | ||||||||||||
| assertEquals(expected, backoff, | ||||||||||||
| String.format("Attempt %d: with jitter=1.0, backoff should be %d ms", attemptNumber, expected)); | ||||||||||||
| } | ||||||||||||
| } finally { | ||||||||||||
| ExponentialBackoff.clearTestJitterSupplier(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Test with jitter = 0, all backoffs should be 0 | ||||||||||||
| ExponentialBackoff.setTestJitterSupplier(() -> 0.0); | ||||||||||||
| try { | ||||||||||||
| for (int attemptNumber = 1; attemptNumber < EXPECTED_BACKOFFS_MAX_VALUES.length; attemptNumber++) { | ||||||||||||
| long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber); | ||||||||||||
| assertEquals(0, backoff, "With jitter=0, backoff should always be 0 ms"); | ||||||||||||
| } | ||||||||||||
| } finally { | ||||||||||||
| ExponentialBackoff.clearTestJitterSupplier(); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+55
to
+80
Member
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.
Let's remove the comments and use more proper tools to structure the code. |
||||||||||||
| } | ||||||||||||
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
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.
Let's remove this new method. See #1899 (comment) for the details.