-
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
9676ff0
9153831
9eebf51
0c10988
8b24709
60acf51
94a79db
d4bc4c7
11786dd
43dab53
69630b0
ae3f140
2f381d6
4a3d1ae
00cd332
dbc7511
b067862
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,76 @@ | ||
| /* | ||
| * 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 { | ||
|
|
||
| private static final double TRANSACTION_BASE_MS = 5.0; | ||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| static final double TRANSACTION_MAX_MS = 500.0; | ||
| private static final double TRANSACTION_GROWTH = 1.5; | ||
|
|
||
| // TODO-JAVA-6079 | ||
| private static DoubleSupplier testJitterSupplier = null; | ||
nhachicha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| private ExponentialBackoff() { | ||
| } | ||
|
|
||
| /** | ||
| * Calculate the backoff in milliseconds for transaction retries. | ||
| * | ||
| * @param attemptNumber attempt number > 0 | ||
| * @return The calculated backoff in milliseconds. | ||
| */ | ||
| public static long calculateTransactionBackoffMs(final int attemptNumber) { | ||
| assertTrue(attemptNumber > 0, "Attempt number must be at least 1 (1-based) in the context of transaction backoff calculation"); | ||
| double jitter = testJitterSupplier != null | ||
| ? testJitterSupplier.getAsDouble() | ||
nhachicha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| : 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. | ||
| */ | ||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| public static void setTestJitterSupplier(final DoubleSupplier supplier) { | ||
| testJitterSupplier = supplier; | ||
| } | ||
nhachicha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * 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(); | ||
| } | ||
| } |
stIncMale marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /* | ||
| * 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 java.util.function.DoubleSupplier; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| class ExponentialBackoffTest { | ||
| /** | ||
| * Expected {@linkplain ExponentialBackoff#calculateTransactionBackoffMs(int) backoffs} with 1.0 as | ||
| * {@link ExponentialBackoff#setTestJitterSupplier(DoubleSupplier) jitter}. | ||
| */ | ||
| 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() { | ||
| 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 between 0 ms and %d ms, got: %d", attemptNumber, | ||
| expectedBackoff, backoff)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void testCalculateTransactionBackoffMsRespectsMaximum() { | ||
| for (int attemptNumber = 1; attemptNumber < EXPECTED_BACKOFFS_MAX_VALUES.length * 2; attemptNumber++) { | ||
| 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
+46
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.
Collaborator
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. removed |
||
| attemptNumber, ExponentialBackoff.TRANSACTION_MAX_MS, backoff)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void testCustomJitterWithOne() { | ||
| 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 | ||
| void testCustomJitterWithZero() { | ||
| 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
+81
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.
Collaborator
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. split into two tests |
||
| } | ||
This file was deleted.
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.
Please have a look at the following review from Claude, and address relevant items. Start at the bottom with the action item summary. (Report is AI generated, as with copilot.)
https://github.com/mongodb/mongo-java-driver/blob/ad34228e32af311e31a52e04fff7b3d21626935f/review.md
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.
Fixed
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.
Could you please:
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.
Output using Claude Opus 4.6 review
Verification Results — review.md Issues vs Current Codebase
Fixed (15 issues)
[0,1)→[0,1][0, 1]attempt number > 0with clear assertion message% dspacing"%d ms"clearTransactionContextOnErrorapplyMajorityWriteConcernbeforecontinue outercontinuewithin commit-retry loop, nocontinue outertimeoutExceptionacceptsThrowablewrapInMongoTimeoutException, acceptsMongoExceptionhasTimeoutMStimeoutMsConfiguredTimeoutContext(TimeoutSettings, Timeout)constructortimeoutOrAlternative(Timeout)unnecessaryTimeout timeoutOrAlternative(Timeout)method was removed. The currentlong timeoutOrAlternative(long)is a different method withExponentialBackoffTestispublic-4code, missingassertInstanceOf/assertSame)withTransactionstructure doesn't follow specPartially Fixed (1 issue)
wrapInMongoTimeoutExceptionatClientSessionImpl.java:414explicitly copies labels viacause.getErrorLabels().forEach(timeoutException::addLabel). Not fixed at the constructor level:MongoException(String, Throwable)still does not copy labels, unlike `MongoException(int, String,which does. The reviewer's broader architectural concern about constructor inconsistency remains open, but thewithTransaction` path is correct.Deferred (1 issue)
bb9dddd8, needs≥ 1125200e)TODO-BACKPRESSUREin [PR≥ 1125200eafter merging this PR intobackpressure. Not a blocker for this PR.Summary
Claude Review code diff only then with content and comment
## Remaining Issues to AddresstestJitterSuppliershould bevolatile— mutable static read/written across threads without memory visibility guaranteeExponentialBackoff.javawithTransactioncode doesn't clearly follow the spec algorithm — hard to verify correctness against the specification's prose stepsClientSessionImpl.javafd951e9not yet cherry-pickedapplyMajorityWriteConcernToTransactionOptionscalled on outer (transaction) retry — should only be called on commit retry; may spoil write concern for next transaction attemptClientSessionImpl.java(commit retry loop)fd951e9but not cherry-pickedTODO-BACKPRESSUREcomments in production code — must be resolved before mergingbackpressure→mainMongoException.java,ExponentialBackoff.javatransactions-convenient-apiJSON tests not includedtesting/resources/specifications(submodule)mainbackpressurefd951e9) not integrated — contains fixes for items 2, 3, 4 plus removal of unnecessarytimeoutOrAlternative, cleaner variable namingtimeoutMsConfigured), andMongoTimeoutExceptionno-double-wrap logicClientSessionImpl.java,TimeoutContext.javawithTransaction surfaces a timeout after exhausting transient transaction retriesis runTODO-BACKPRESSURE ValentinWithTransactionProseTest.javafinalizevsfinishvsstop)ClientSessionImpl.javatracing codePriority Summary
backpressure: Items 7 (integrate @stIncMale's refactor), 8, 9main: Items 5 (TODOs), 6 (submodule)Copilot happy after 5 rounds (mostly false positives or nit comments)
Round 1
Round 2
Round 3
Round 4
Round 5