-
Notifications
You must be signed in to change notification settings - Fork 1.5k
JAVA-5950 Update Transactions Convenient API with exponential backoff on retries #1852
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
bd98229
72cd714
573c86c
ecf9b4d
68dbece
7cfc4c4
cd96a49
82dc8b1
558bfad
fbe881e
d89c714
2eccff0
c406ae7
a28ce98
27ddef9
3bef60a
b051701
90ec4d5
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,175 @@ | ||||||||
| /* | ||||||||
| * 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; | ||||||||
|
|
||||||||
| import com.mongodb.annotations.NotThreadSafe; | ||||||||
|
|
||||||||
| import java.util.concurrent.ThreadLocalRandom; | ||||||||
| import java.util.function.DoubleSupplier; | ||||||||
|
|
||||||||
| import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE; | ||||||||
|
|
||||||||
| /** | ||||||||
| * Implements exponential backoff with jitter for retry scenarios. | ||||||||
| * Formula: delayMS = jitter * min(maxBackoffMs, baseBackoffMs * growthFactor^retryCount) | ||||||||
| * where jitter is random value [0, 1). | ||||||||
|
Comment on lines
+28
to
+29
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 formula, as well as the constants are internal detail, irrelevant even to the internal user of this internal API. Let's remove this. |
||||||||
| * | ||||||||
| * <p>This class provides factory methods for common use cases: | ||||||||
| * <ul> | ||||||||
| * <li>{@link #forTransactionRetry()} - For withTransaction retries (5ms base, 500ms max, 1.5 growth)</li> | ||||||||
| * <li>{@link #forCommandRetry()} - For command retries with overload (100ms base, 10000ms max, 2.0 growth)</li> | ||||||||
|
Comment on lines
+33
to
+34
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 not duplicate method-level documentation in the class-level documentation. Any duplication opens up opportunities for inconsistencies. We duplicate public API documentation for the sake of user convenience (though I has always been opposed to that, because it have lead to inconsistencies, and will lead to them again, which harm users), but internally, there seem to be no reason to duplicate documentation at all. |
||||||||
| * </ul> | ||||||||
| */ | ||||||||
| @NotThreadSafe | ||||||||
| public final class ExponentialBackoff { | ||||||||
| // Transaction retry constants (per spec) | ||||||||
| private static final double TRANSACTION_BASE_BACKOFF_MS = 5.0; | ||||||||
| private static final double TRANSACTION_MAX_BACKOFF_MS = 500.0; | ||||||||
| private static final double TRANSACTION_BACKOFF_GROWTH = 1.5; | ||||||||
|
|
||||||||
| // Command retry constants (per spec) | ||||||||
| private static final double COMMAND_BASE_BACKOFF_MS = 100.0; | ||||||||
| private static final double COMMAND_MAX_BACKOFF_MS = 10000.0; | ||||||||
| private static final double COMMAND_BACKOFF_GROWTH = 2.0; | ||||||||
|
Comment on lines
+39
to
+47
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.
|
||||||||
|
|
||||||||
| private final double baseBackoffMs; | ||||||||
| private final double maxBackoffMs; | ||||||||
| private final double growthFactor; | ||||||||
| private int retryCount = 0; | ||||||||
|
|
||||||||
| // Test-only jitter supplier - when set, overrides ThreadLocalRandom | ||||||||
| private static volatile DoubleSupplier testJitterSupplier = null; | ||||||||
|
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. It is better to comment on a private field by writing a documentation comment for the field. However here, the comment not only does not add anything of value to the reader, but actually confuses him: I had to look at the usage of the field to understand what this comment says; and this is likely to happen to any reader because no one knows what
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.
If we actually had |
||||||||
|
|
||||||||
| /** | ||||||||
| * Creates an exponential backoff instance with specified parameters. | ||||||||
| * | ||||||||
| * @param baseBackoffMs Initial backoff in milliseconds | ||||||||
| * @param maxBackoffMs Maximum backoff cap in milliseconds | ||||||||
| * @param growthFactor Exponential growth factor (e.g., 1.5 or 2.0) | ||||||||
| */ | ||||||||
| public ExponentialBackoff(final double baseBackoffMs, final double maxBackoffMs, final double growthFactor) { | ||||||||
|
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. There is no reason to expose this constructor. It should be |
||||||||
| this.baseBackoffMs = baseBackoffMs; | ||||||||
| this.maxBackoffMs = maxBackoffMs; | ||||||||
| this.growthFactor = growthFactor; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Creates a backoff instance configured for withTransaction retries. | ||||||||
| * Uses: 5ms base, 500ms max, 1.5 growth factor. | ||||||||
|
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. If you insist that documenting this internally is valuable, which I doubt, let's do that in a way that prevents the documentation and the actual values from becoming inconsistent:
Suggested change
P.S. Notice that I used a space between the numerical value and unit symbol. This is in accordance with the "5.4.3 Formatting the value of a quantity" section in The International System of Units (SI) Brochure released by The International Bureau of Weights and Measures (BIPM). |
||||||||
| * | ||||||||
| * @return ExponentialBackoff configured for transaction retries | ||||||||
| */ | ||||||||
| public static ExponentialBackoff forTransactionRetry() { | ||||||||
| return new ExponentialBackoff( | ||||||||
| TRANSACTION_BASE_BACKOFF_MS, | ||||||||
| TRANSACTION_MAX_BACKOFF_MS, | ||||||||
| TRANSACTION_BACKOFF_GROWTH | ||||||||
| ); | ||||||||
| } | ||||||||
|
Comment on lines
+76
to
+82
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. Within this PR, I failed to find any benefits of expressing the backoff computation as behavior of an object (an instance of If in the future we observe that this is not enough and the "object" approach is needed, we'll be able to change the approach. But we will have a clear reason for that. Also, storing all the constants in each instance of P.S. Some other review comments I left are written within the current "object" approach (as opposed to he proposed "method" approach). If the suggestion in this comment is applied, those comments should not be automatically discarded, but rather each should be examined and adopted, if applicable, to the "method" approach. 1 If in the future we need another |
||||||||
|
|
||||||||
| /** | ||||||||
| * Creates a backoff instance configured for command retries during overload. | ||||||||
| * Uses: 100ms base, 10000ms max, 2.0 growth factor. | ||||||||
| * | ||||||||
| * @return ExponentialBackoff configured for command retries | ||||||||
| */ | ||||||||
| public static ExponentialBackoff forCommandRetry() { | ||||||||
|
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 not introduce unused code that we foresee to be needed in the future. In this PR, we only need backoffs for the convenient transaction API. |
||||||||
| return new ExponentialBackoff( | ||||||||
| COMMAND_BASE_BACKOFF_MS, | ||||||||
| COMMAND_MAX_BACKOFF_MS, | ||||||||
| COMMAND_BACKOFF_GROWTH | ||||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Calculate next backoff delay with jitter. | ||||||||
| * | ||||||||
| * @return delay in milliseconds | ||||||||
| */ | ||||||||
| public long calculateDelayMs() { | ||||||||
|
Comment on lines
+98
to
+103
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 rename the method to |
||||||||
| // Use test jitter supplier if set, otherwise use ThreadLocalRandom | ||||||||
| double jitter = testJitterSupplier != null | ||||||||
| ? testJitterSupplier.getAsDouble() | ||||||||
| : ThreadLocalRandom.current().nextDouble(); | ||||||||
|
Comment on lines
+104
to
+107
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. This comment simply repeats what the trivial expression it is about does. Such comments have pollute the code without benefitting us in any way. Let's remove it. |
||||||||
| double exponentialBackoff = baseBackoffMs * Math.pow(growthFactor, retryCount); | ||||||||
| double cappedBackoff = Math.min(exponentialBackoff, maxBackoffMs); | ||||||||
| retryCount++; | ||||||||
| return Math.round(jitter * cappedBackoff); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Set a custom jitter supplier for testing purposes. | ||||||||
| * This overrides the default ThreadLocalRandom jitter generation. | ||||||||
|
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. This class using P.S. The majority of the internal docs written for this trivial class seem to be not useful. But it takes up the resources when reviewing, and also when producing it (assuming it was produced manually). |
||||||||
| * | ||||||||
| * @param supplier A DoubleSupplier that returns values in [0, 1) range, or null to use default | ||||||||
| */ | ||||||||
| @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; | ||||||||
| } | ||||||||
|
Comment on lines
+114
to
+131
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. Such global states and methods allowing us to fiddle with the behavior for testing purposes is a dangerous and obscure way of programming. This approach reminds me about https://pvs-studio.com/en/blog/posts/0439/. Here are some quotes from there:
I know we introduced a similar thing in CSOT: Instead, we should try and do what was done in
When the above is done, the new The proposed approach will also enable us to get rid of |
||||||||
|
|
||||||||
| /** | ||||||||
| * Reset retry counter for new sequence of retries. | ||||||||
| */ | ||||||||
| public void reset() { | ||||||||
| retryCount = 0; | ||||||||
| } | ||||||||
|
Comment on lines
+133
to
+138
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. This method is not used, let's remove it. |
||||||||
|
|
||||||||
| /** | ||||||||
| * Get current retry count for testing. | ||||||||
| * | ||||||||
| * @return current retry count | ||||||||
| */ | ||||||||
| public int getRetryCount() { | ||||||||
| return retryCount; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Get the base backoff in milliseconds. | ||||||||
| * | ||||||||
| * @return base backoff | ||||||||
| */ | ||||||||
| public double getBaseBackoffMs() { | ||||||||
| return baseBackoffMs; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Get the maximum backoff in milliseconds. | ||||||||
| * | ||||||||
| * @return maximum backoff | ||||||||
| */ | ||||||||
| public double getMaxBackoffMs() { | ||||||||
| return maxBackoffMs; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Get the growth factor. | ||||||||
| * | ||||||||
| * @return growth factor | ||||||||
| */ | ||||||||
| public double getGrowthFactor() { | ||||||||
| return growthFactor; | ||||||||
| } | ||||||||
|
Comment on lines
+140
to
+174
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. At the very least, we should make these methods package-access, and annotate with |
||||||||
| } | ||||||||
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.
Maybe the existing
com.mongodb.internal.timepackage is a better place forExponentialBackoff?