Skip to content

Commit e5ae458

Browse files
nhachichanhachicha
committed
- PR feedback
- Adding logic for DRIVERS-3391
1 parent fda498c commit e5ae458

6 files changed

Lines changed: 77 additions & 79 deletions

File tree

‎driver-core/src/main/com/mongodb/internal/time/ExponentialBackoff.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,12 @@
3030
public final class ExponentialBackoff {
3131

3232
// Constants for transaction retry backoff
33-
private static final double TRANSACTION_BASE_MS = 5.0;
34-
private static final double TRANSACTION_MAX_MS = 500.0;
35-
private static final double TRANSACTION_GROWTH = 1.5;
33+
@VisibleForTesting(otherwise = PRIVATE)
34+
static final double TRANSACTION_BASE_MS = 5.0;
35+
@VisibleForTesting(otherwise = PRIVATE)
36+
static final double TRANSACTION_MAX_MS = 500.0;
37+
@VisibleForTesting(otherwise = PRIVATE)
38+
static final double TRANSACTION_GROWTH = 1.5;
3639

3740
// TODO-JAVA-6079
3841
private static DoubleSupplier testJitterSupplier = null;

‎driver-core/src/test/unit/com/mongodb/internal/time/ExponentialBackoffTest.java‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,43 +22,44 @@
2222
import static org.junit.jupiter.api.Assertions.assertTrue;
2323

2424
public class ExponentialBackoffTest {
25+
// Expected backoffs with jitter=1.0 and growth factor ExponentialBackoff.TRANSACTION_GROWTH
26+
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,
27+
192.21679688, 288.32519531, 432.48779297, 500.0};
2528

2629
@Test
27-
void testTransactionRetryBackoff() {
28-
// Test that the backoff sequence follows the expected pattern with growth factor 1.5
30+
void testCalculateTransactionBackoffMs() {
31+
// Test that the backoff sequence follows the expected pattern with growth factor ExponentialBackoff.TRANSACTION_GROWTH
2932
// Expected sequence (without jitter): 5, 7.5, 11.25, ...
3033
// With jitter, actual values will be between 0 and these maxima
31-
double[] expectedMaxValues = {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};
3234

33-
for (int attemptNumber = 1; attemptNumber <= expectedMaxValues.length; attemptNumber++) {
35+
for (int attemptNumber = 1; attemptNumber <= EXPECTED_BACKOFFS_MAX_VALUES.length; attemptNumber++) {
3436
long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber);
35-
assertTrue(backoff >= 0 && backoff <= Math.round(expectedMaxValues[attemptNumber - 1]),
37+
long expectedBackoff = Math.round(EXPECTED_BACKOFFS_MAX_VALUES[attemptNumber - 1]);
38+
assertTrue(backoff >= 0 && backoff <= expectedBackoff,
3639
String.format("Attempt %d: backoff should be 0-%d ms, got: %d", attemptNumber,
37-
Math.round(expectedMaxValues[attemptNumber - 1]), backoff));
40+
expectedBackoff, backoff));
3841
}
3942
}
4043

4144
@Test
42-
void testTransactionRetryBackoffRespectsMaximum() {
43-
// Even at high attempt numbers, backoff should never exceed 500ms
45+
void testCalculateTransactionBackoffMsRespectsMaximum() {
46+
// Even at high attempt numbers, backoff should never exceed ExponentialBackoff.TRANSACTION_MAX_MS
4447
for (int attemptNumber = 1; attemptNumber < 26; attemptNumber++) {
4548
long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber);
46-
assertTrue(backoff >= 0 && backoff <= 500,
47-
String.format("Attempt %d: backoff should be capped at 500 ms, got: %d ms", attemptNumber, backoff));
49+
assertTrue(backoff >= 0 && backoff <= ExponentialBackoff.TRANSACTION_MAX_MS,
50+
String.format("Attempt %d: backoff should be capped at %f ms, got: %d ms",
51+
attemptNumber, ExponentialBackoff.TRANSACTION_MAX_MS, backoff));
4852
}
4953
}
5054

5155
@Test
5256
void testCustomJitter() {
53-
// Expected backoffs with jitter=1.0 and growth factor 1.5
54-
double[] expectedBackoffs = {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};
55-
5657
// Test with jitter = 1.0
5758
ExponentialBackoff.setTestJitterSupplier(() -> 1.0);
5859
try {
59-
for (int attemptNumber = 1; attemptNumber <= expectedBackoffs.length; attemptNumber++) {
60+
for (int attemptNumber = 1; attemptNumber <= EXPECTED_BACKOFFS_MAX_VALUES.length; attemptNumber++) {
6061
long backoff = ExponentialBackoff.calculateTransactionBackoffMs(attemptNumber);
61-
long expected = Math.round(expectedBackoffs[attemptNumber - 1]);
62+
long expected = Math.round(EXPECTED_BACKOFFS_MAX_VALUES[attemptNumber - 1]);
6263
assertEquals(expected, backoff,
6364
String.format("Attempt %d: with jitter=1.0, backoff should be %d ms", attemptNumber, expected));
6465
}

‎driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/ClientSideOperationTimeoutProseTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.mongodb.client.model.changestream.FullDocument;
2727
import com.mongodb.event.CommandFailedEvent;
2828
import com.mongodb.event.CommandStartedEvent;
29+
import com.mongodb.internal.time.ExponentialBackoff;
2930
import com.mongodb.reactivestreams.client.gridfs.GridFSBucket;
3031
import com.mongodb.reactivestreams.client.gridfs.GridFSBuckets;
3132
import com.mongodb.reactivestreams.client.syncadapter.SyncGridFSBucket;
@@ -578,13 +579,15 @@ private void assertOnlyOneCommandTimeoutFailure(final String command) {
578579
public void setUp() {
579580
super.setUp();
580581
SyncMongoClient.enableSleepAfterSessionClose(postSessionCloseSleep());
582+
ExponentialBackoff.setTestJitterSupplier(() -> 0);
581583
}
582584

583585
@Override
584586
@AfterEach
585587
public void tearDown() throws InterruptedException {
586588
super.tearDown();
587589
SyncMongoClient.disableSleep();
590+
ExponentialBackoff.clearTestJitterSupplier();
588591
}
589592

590593
@Override

‎driver-sync/src/main/com/mongodb/client/internal/ClientSessionImpl.java‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.mongodb.MongoExecutionTimeoutException;
2323
import com.mongodb.MongoInternalException;
2424
import com.mongodb.MongoOperationTimeoutException;
25+
import com.mongodb.MongoTimeoutException;
2526
import com.mongodb.ReadConcern;
2627
import com.mongodb.TransactionOptions;
2728
import com.mongodb.WriteConcern;
@@ -51,6 +52,7 @@
5152
import static com.mongodb.assertions.Assertions.assertTrue;
5253
import static com.mongodb.assertions.Assertions.isTrue;
5354
import static com.mongodb.assertions.Assertions.notNull;
55+
import static com.mongodb.internal.TimeoutContext.createMongoTimeoutException;
5456
import static com.mongodb.internal.thread.InterruptionUtil.interruptAndCreateMongoInterruptedException;
5557

5658
final class ClientSessionImpl extends BaseClientSessionImpl implements ClientSession {
@@ -256,6 +258,7 @@ public <T> T withTransaction(final TransactionBody<T> transactionBody) {
256258
public <T> T withTransaction(final TransactionBody<T> transactionBody, final TransactionOptions options) {
257259
notNull("transactionBody", transactionBody);
258260
TimeoutContext withTransactionTimeoutContext = createTimeoutContext(options);
261+
final boolean hasTimeoutMS = withTransactionTimeoutContext.hasTimeoutMS();
259262
Timeout withTransactionTimeout = withTransactionTimeoutContext.timeoutOrAlternative(
260263
assertNotNull(TimeoutContext.startTimeout(MAX_RETRY_TIME_LIMIT_MS)));
261264
BooleanSupplier withTransactionTimeoutExpired = () -> withTransactionTimeout.call(TimeUnit.MILLISECONDS,
@@ -267,7 +270,7 @@ public <T> T withTransaction(final TransactionBody<T> transactionBody, final Tra
267270
outer:
268271
while (true) {
269272
if (transactionAttempt > 0) {
270-
backoff(transactionAttempt, withTransactionTimeout, assertNotNull(lastError));
273+
backoff(transactionAttempt, withTransactionTimeout, assertNotNull(lastError), hasTimeoutMS);
271274
}
272275
T retVal;
273276
try {
@@ -286,12 +289,15 @@ public <T> T withTransaction(final TransactionBody<T> transactionBody, final Tra
286289
lastError = (MongoException) e;
287290
if (!(e instanceof MongoOperationTimeoutException)) {
288291
MongoException exceptionToHandle = OperationHelper.unwrap((MongoException) e);
289-
if (exceptionToHandle.hasErrorLabel(TRANSIENT_TRANSACTION_ERROR_LABEL)
290-
&& !withTransactionTimeoutExpired.getAsBoolean()) {
291-
if (transactionSpan != null) {
292-
transactionSpan.spanFinalizing(false);
292+
if (exceptionToHandle.hasErrorLabel(TRANSIENT_TRANSACTION_ERROR_LABEL)) {
293+
if (withTransactionTimeoutExpired.getAsBoolean()) {
294+
throw timeoutException(hasTimeoutMS, e);
295+
} else {
296+
if (transactionSpan != null) {
297+
transactionSpan.spanFinalizing(false);
298+
}
299+
continue;
293300
}
294-
continue;
295301
}
296302
}
297303
}
@@ -305,18 +311,22 @@ public <T> T withTransaction(final TransactionBody<T> transactionBody, final Tra
305311
} catch (MongoException e) {
306312
lastError = e;
307313
clearTransactionContextOnError(e);
308-
if (!(e instanceof MongoOperationTimeoutException)
309-
&& !withTransactionTimeoutExpired.getAsBoolean()) {
310-
applyMajorityWriteConcernToTransactionOptions();
311-
312-
if (!(e instanceof MongoExecutionTimeoutException)
313-
&& e.hasErrorLabel(UNKNOWN_TRANSACTION_COMMIT_RESULT_LABEL)) {
314-
continue;
315-
} else if (e.hasErrorLabel(TRANSIENT_TRANSACTION_ERROR_LABEL)) {
316-
if (transactionSpan != null) {
317-
transactionSpan.spanFinalizing(true);
314+
if (!(e instanceof MongoOperationTimeoutException)) {
315+
if (withTransactionTimeoutExpired.getAsBoolean()) {
316+
throw timeoutException(hasTimeoutMS, e);
317+
318+
} else {
319+
applyMajorityWriteConcernToTransactionOptions();
320+
321+
if (!(e instanceof MongoExecutionTimeoutException)
322+
&& e.hasErrorLabel(UNKNOWN_TRANSACTION_COMMIT_RESULT_LABEL)) {
323+
continue;
324+
} else if (e.hasErrorLabel(TRANSIENT_TRANSACTION_ERROR_LABEL)) {
325+
if (transactionSpan != null) {
326+
transactionSpan.spanFinalizing(true);
327+
}
328+
continue outer;
318329
}
319-
continue outer;
320330
}
321331
}
322332
throw e;
@@ -381,10 +391,10 @@ private TimeoutContext createTimeoutContext(final TransactionOptions transaction
381391
}
382392

383393
private static void backoff(final int transactionAttempt,
384-
final Timeout withTransactionTimeout, final MongoException lastError) {
394+
final Timeout withTransactionTimeout, final MongoException lastError, final boolean hasTimeoutMS) {
385395
long backoffMs = ExponentialBackoff.calculateTransactionBackoffMs(transactionAttempt);
386396
withTransactionTimeout.shortenBy(backoffMs, TimeUnit.MILLISECONDS).onExpired(() -> {
387-
throw lastError;
397+
throw timeoutException(hasTimeoutMS, lastError);
388398
});
389399
try {
390400
if (backoffMs > 0) {
@@ -394,4 +404,10 @@ private static void backoff(final int transactionAttempt,
394404
throw interruptAndCreateMongoInterruptedException("Transaction retry interrupted", e);
395405
}
396406
}
407+
408+
private static MongoException timeoutException(final boolean hasTimeoutMS, final Throwable cause) {
409+
return hasTimeoutMS
410+
? createMongoTimeoutException(cause) // CSOT timeout exception
411+
: new MongoTimeoutException("Operation exceeded the timeout limit", cause); // Legacy timeout exception
412+
}
397413
}

‎driver-sync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@
5151
import com.mongodb.internal.connection.ServerHelper;
5252
import com.mongodb.internal.connection.TestCommandListener;
5353
import com.mongodb.internal.connection.TestConnectionPoolListener;
54-
import com.mongodb.internal.time.ExponentialBackoff;
5554
import com.mongodb.test.FlakyTest;
5655
import org.bson.BsonDocument;
5756
import org.bson.BsonInt32;
@@ -1121,11 +1120,6 @@ public void setUp() {
11211120
chunksCollectionHelper = new CollectionHelper<>(new BsonDocumentCodec(), gridFsChunksNamespace);
11221121
commandListener = new TestCommandListener();
11231122

1124-
if (!isAsync()) {
1125-
// setting jitter to 0 to make test using withTransaction deterministic (i.e retries immediately) otherwise we might get
1126-
// MongoCommandException setup in the failpoint instead of MongoOperationTimeoutException depending on the random jitter value.
1127-
ExponentialBackoff.setTestJitterSupplier(() -> 0);
1128-
}
11291123
}
11301124

11311125
@AfterEach
@@ -1151,10 +1145,6 @@ public void tearDown() throws InterruptedException {
11511145
//noinspection ResultOfMethodCallIgnored
11521146
executor.awaitTermination(MAX_VALUE, NANOSECONDS);
11531147
}
1154-
1155-
if (!isAsync()) {
1156-
ExponentialBackoff.clearTestJitterSupplier();
1157-
}
11581148
}
11591149

11601150
@AfterAll

‎driver-sync/src/test/functional/com/mongodb/client/WithTransactionProseTest.java‎

Lines changed: 17 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -219,44 +219,17 @@ public void testTimeoutMSAndLegacySettings() {
219219
@DisplayName("Retry Backoff is Enforced")
220220
@Test
221221
public void testRetryBackoffIsEnforced() throws InterruptedException {
222-
// Run with jitter = 0 (no backoff)
223-
ExponentialBackoff.setTestJitterSupplier(() -> 0.0);
224-
225-
BsonDocument failPointDocument = BsonDocument.parse("{'configureFailPoint': 'failCommand', 'mode': {'times': 13}, "
226-
+ "'data': {'failCommands': ['commitTransaction'], 'errorCode': 251}}");
227-
228-
long noBackoffTime;
229-
try (ClientSession session = client.startSession();
230-
FailPoint ignored = FailPoint.enable(failPointDocument, getPrimary())) {
231-
StartTime startTime = StartTime.now();
232-
session.withTransaction(() -> collection.insertOne(session, Document.parse("{}")));
233-
noBackoffTime = startTime.elapsed().toMillis();
234-
} finally {
235-
// Clear the test jitter supplier to avoid affecting other tests
236-
ExponentialBackoff.clearTestJitterSupplier();
237-
}
238-
239-
// Run with jitter = 1 (full backoff)
240-
ExponentialBackoff.setTestJitterSupplier(() -> 1.0);
241-
242-
failPointDocument = BsonDocument.parse("{'configureFailPoint': 'failCommand', 'mode': {'times': 13}, "
222+
final BsonDocument failPointDocument = BsonDocument.parse("{'configureFailPoint': 'failCommand', 'mode': {'times': 13}, "
243223
+ "'data': {'failCommands': ['commitTransaction'], 'errorCode': 251}}");
244224

245-
long withBackoffTime;
246-
try (ClientSession session = client.startSession();
247-
FailPoint ignored = FailPoint.enable(failPointDocument, getPrimary())) {
248-
StartTime startTime = StartTime.now();
249-
session.withTransaction(() -> collection.insertOne(session, Document.parse("{}")));
250-
withBackoffTime = startTime.elapsed().toMillis();
251-
} finally {
252-
ExponentialBackoff.clearTestJitterSupplier();
253-
}
225+
long noBackoffTime = measureTransactionLatency(0.0, failPointDocument);
226+
long withBackoffTime = measureTransactionLatency(1.0, failPointDocument);
254227

255228
long expectedWithBackoffTime = noBackoffTime + 1800;
256229
long actualDifference = Math.abs(withBackoffTime - expectedWithBackoffTime);
257230

258-
assertTrue(actualDifference < 1000, String.format("Expected withBackoffTime to be ~% dms (noBackoffTime %d ms + 1800 ms), but"
259-
+ " got %d ms. Difference: %d ms (tolerance: 1000 ms per spec)", expectedWithBackoffTime, noBackoffTime, withBackoffTime,
231+
assertTrue(actualDifference < 500, String.format("Expected withBackoffTime to be ~% dms (noBackoffTime %d ms + 1800 ms), but"
232+
+ " got %d ms. Difference: %d ms (tolerance: 500 ms per spec)", expectedWithBackoffTime, noBackoffTime, withBackoffTime,
260233
actualDifference));
261234
}
262235

@@ -282,6 +255,18 @@ public void testExponentialBackoffOnTransientError() throws InterruptedException
282255
}
283256
}
284257

258+
private long measureTransactionLatency(final double jitter, final BsonDocument failPointDocument) throws InterruptedException {
259+
ExponentialBackoff.setTestJitterSupplier(() -> jitter);
260+
try (ClientSession session = client.startSession();
261+
FailPoint ignored = FailPoint.enable(failPointDocument, getPrimary())) {
262+
StartTime startTime = StartTime.now();
263+
session.withTransaction(() -> collection.insertOne(session, Document.parse("{}")));
264+
return startTime.elapsed().toMillis();
265+
} finally {
266+
ExponentialBackoff.clearTestJitterSupplier();
267+
}
268+
}
269+
285270
private static boolean canRunTests() {
286271
return isSharded() || isDiscoverableReplicaSet();
287272
}

0 commit comments

Comments
 (0)