JAVA-4034 Refactor read/write retries to accommodate for both `PoolClearedError` and CSOT #782

stIncMale · 2021-09-07T23:53:50Z

The baseline commit for this PR is the last commit in #701. As a consequence, I had to push the stIncMale:JAVA-3928 branch to mongodb/mongo-java-driver to be able to create a PR with diffs relevant to JAVA-4034. Once this PR is reviewed and approved, I will:

merge JAVA-3928 Make DefaultConnectionPool pausable #701;
change the target branch of this PR to master
merge this PR
delete mongodb:JAVA-3928.

Changes in this commit do the following two major things:

Selecting a server (binding.get*ConnectionSource)
and checking out a connection (source.getConnection) is now part of a retryable operation.
Such a change makes MongoConnectionPoolClearedExceptions part of the operation,
thus allowing us to retry the operation when the exception happens.
The retry limit (not more than two attempts) was hardcoded in the driver's code structure,
i.e., the code was written in such a way that it was describing the first attempt, then depending
on the outcome, the code for the second attempt was executed, with the code for the second attempt
mostly duplicating the code for the first attempt, but having different error handling. Such an approach
not only negatively affects the code readability, but also prevents changing the number of attempts,
let alone making decisions on whether to retry based not on an attempt limit.
The pseudocode in the specifications is also written this way, and at least some other drivers,
e.g., C#, Rust, took a similar approach to structure the code.

Changes in this commit represent the outcome of refactoring the code related to the retry logic.
In the new code the retry logic is decoupled from the logic of a retryable operation as much as possible.
Since our operations are quite complex and may decide to break retrying in the middle of an attempt,
e.g., because the server selected in the attempt does not support retries, the operation logic
is still aware of the fact that it may be retried. However, it is important to understand that
this awareness is a direct consequence of the business logic. It cannot be gotten rid of
regardless of the approach taken to structure the code. Operations that have simpler business logic
can be written in a retry-agnostic way without making changes to the retry framework
that was added as part of this commit.

With the changes in this commit, applying the client-side timeout (CSOT) to a retryable operation
is as simple as replacing the hard retry limit condition with a condition that checks
whether there is time left for attempting the operation again.

Here are also a few interesting stats:

the line count in CommandOperationHelper changed from 865 to 620
the line count in OperationHelper changed from 768 to 734
the line count in MixedBulkWriteOperation stayed roughly the same (changed from 553 to 551, but a portion of it is comments, while previously it was all code)

Given that the business logic became more complicated due to the item #1 described above, I consider the above reduction in the amount of code expressing the business logic a very good thing, especially given that it is accompanied by the code (particularly callback-based) being made more linear and requiring almost no changes to support CSOT.

JAVA-4034

stIncMale · 2021-09-08T02:07:15Z

Since the target brach is not master, the tests are not run automatically. Here are the results of all our tests corresponding to the commit 2c08fd1b1.

P.S. The atlas-test task often fails with CertificateExpiredException. This has been happening since the beginning of August.
P.P.S. The serverless-test task times out, which is also the case for the master branch.

stIncMale · 2021-09-14T15:25:02Z

driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java

+                    bulkWriteTracker.batch().ifPresent(bulkWriteBatch -> {
+                        assertTrue(prospectiveFailedResult instanceof MongoWriteConcernWithResponseException);
+                        bulkWriteBatch.addResult((BsonDocument) ((MongoWriteConcernWithResponseException) prospectiveFailedResult)
+                                .getResponse());
+                        BulkWriteTracker.attachNext(retryState, bulkWriteBatch);
+                    });


This piece of code seems to be dead from the standpoint of the tests we have, i.e., the breakAndThrowIfRetryAnd always breaks retrying in tests when it is executed. The same is true for executeSync.

The rest of the code seem to be covered.

rozza

I really like it, mostly nits and a few change requests.

driver-core/src/main/com/mongodb/internal/async/function/AsyncCallbackFunction.java

driver-core/src/main/com/mongodb/internal/async/function/AsyncCallbackBiFunction.java

driver-core/src/main/com/mongodb/internal/async/function/AsyncCallbackSupplier.java

driver-core/src/main/com/mongodb/internal/async/function/LoopState.java

rozza · 2021-09-14T13:08:08Z

driver-core/src/main/com/mongodb/internal/async/function/RetryState.java

+     */
+    public RetryState(final int retries) {
+        assertTrue(retries >= 0);
+        assertTrue(retries < INFINITE_ATTEMPTS);


You could just reuse this constructor and default retries to INFINITE_ATTEMPTS in the no args constructor.

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

driver-core/src/main/com/mongodb/internal/async/function/LoopState.java

driver-core/src/main/com/mongodb/internal/async/function/RetryState.java

stIncMale · 2021-09-23T19:09:04Z

driver-core/src/test/functional/com/mongodb/client/syncadapter/SupplyingCallback.java

+import java.util.function.Supplier;
+
+@ThreadSafe
+public final class SupplyingCallback<R> implements SingleResultCallback<R>, Supplier<R> {


Initially this class was added in the successive PR #784, but I needed it here to write unit tests for LoopState/RetryState. So I had to "back port" the class here, and will pull the changes to #784 to resolve conflicts.

jyemin

This looks fantastic. Just a few nits, and one substantial refactoring request.

driver-core/src/main/com/mongodb/internal/binding/SingleServerBinding.java

driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java

driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java

jyemin

LGTM.

jyemin · 2021-09-24T13:51:28Z

driver-core/src/main/com/mongodb/internal/async/function/LoopState.java

+     *
+     * @see #iteration()
+     */
+    public boolean firstIteration() {


Our convention seems to be to prefix with is even in internal code, so might as well follow that.

rozza

LGTM

driver-core/src/main/com/mongodb/internal/async/function/AsyncCallbackFunction.java

rozza · 2021-09-23T09:40:07Z

driver-core/src/main/com/mongodb/internal/async/function/LoopState.java

+     *
+     * @see #iteration()
+     */
+    public boolean firstIteration() {


Thats good to know, I'll defer to @jyemin but I'm happy either way.

driver-core/src/main/com/mongodb/internal/async/function/RetryState.java

rozza · 2021-09-23T09:50:06Z

driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java

-    static BsonDocument executeCommand(final ReadBinding binding, final String database, final CommandCreator commandCreator,
-                                       final boolean retryReads) {
-        return executeCommand(binding, database, commandCreator, new BsonDocumentCodec(), retryReads);
+    private static Throwable chooseAndMutateRetryableReadException(


It seems confusing as this method doesn't mutate anything. It seems more a kin to a concrete BiFunction implementation.

Is there some expectation in the future from within these methods that an existing exception will be mutated?

driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java

driver-core/src/main/com/mongodb/internal/operation/retry/AttachmentKeys.java

stIncMale · 2021-09-27T18:54:19Z

Apparently, the diff displayed by the GitHub PR UI is calculated not based on the state of the last commits in the "into"/"from" branches, but based on the state of the last commit that is common for the two aforementioned branches, and the last commits in the "from" branch. I thought this is not how the PR diff is calculated. Anyway, this prevents me from simply changing the "into" branch, because the PR diff now combines a diff for both its predecessor #701 and the changes that are actually proposed in the current PR.

In order to redeem the diff for this PR, I will rebase the "from" branch on top of the new "into" (master). This will re-create all the commits in this PR, but at least the diff will be correct again.

…and CSOT Changes in this commit do the following two major things: 1) These changes make selecting a server (`binding.get*ConnectionSource`) and checking out a connection (`source.getConnection`) part of a retryable operation. Such a change makes `MongoConnectionPoolClearedException`s part of the operation, thus allowing us to retry the operation when the exception happens. 2) The retry limit (not more than two attempts) was hardcoded in the driver's code structure, i.e., the code was written in such a way that it was describing the first attempt, then depending on the outcome, the code for the second attempt was executed, with the code for the second attempt mostly duplicating the code for the first attempt, but having different error handling. Such an approach not only negatively affects the code readability, but also prevents changing the number of attempts, let alone making decisions on whether to retry based not on an attempt limit. The pseudocode in the specifications is also written this way, and at least some other drivers, e.g., C#, Rust, took a similar approach to structure the code. Changes in this commit represent the outcome of refactoring the code related to the retry logic. In the new code the retry logic is decoupled from the logic of a retryable operation as much as possible. Since our operations are quite complex and may decide to break retrying in the middle of an attempt, e.g., because the server selected in the attempt does not support retries, the operation logic is still aware of the fact that it may be retried. However, it is important to understand that this awareness is a direct consequence of the business logic. It cannot be gotten rid of regardless of the approach taken to structure the code. Operations that have simpler business logic can be written in a retry-agnostic way without making changes to the retry framework that was added as part of this commit. With the changes in this commit, applying the client-side timeout (CSOT) to a retryable operation is as simple as replacing the hard retry limit condition with a condition that checks whether there is time left for attempting the operation again. JAVA-4034

JAVA-4034

…n while writing JAVA-4034

…rnalException` JAVA-4034

JAVA-4034

stIncMale requested review from jyemin and rozza September 7, 2021 23:55

stIncMale changed the title ~~Refactor read/write retries to accommodate for both PoolClearedError and CSOT~~ JAVA-4034 Refactor read/write retries to accommodate for both PoolClearedError and CSOT Sep 8, 2021

stIncMale changed the title ~~JAVA-4034 Refactor read/write retries to accommodate for both PoolClearedError and CSOT~~ JAVA-4034 Refactor read/write retries to accommodate for both PoolClearedError and CSOT Sep 8, 2021

stIncMale force-pushed the JAVA-4034 branch from 1490a3d to 85d4460 Compare September 9, 2021 01:23

stIncMale commented Sep 14, 2021

View reviewed changes

rozza requested changes Sep 14, 2021

View reviewed changes

stIncMale mentioned this pull request Sep 21, 2021

JAVA-3926 Consider server load during server selection #784

Merged

stIncMale self-assigned this Sep 21, 2021

stIncMale requested a review from rozza September 23, 2021 19:04

stIncMale commented Sep 23, 2021

View reviewed changes

jyemin requested changes Sep 23, 2021

View reviewed changes

stIncMale requested a review from jyemin September 24, 2021 01:25

jyemin approved these changes Sep 24, 2021

View reviewed changes

rozza approved these changes Sep 27, 2021

View reviewed changes

stIncMale changed the base branch from JAVA-3928 to master September 27, 2021 18:43

stIncMale force-pushed the JAVA-4034 branch from 2c08fd1 to 165568a Compare September 27, 2021 19:04

stIncMale added 9 commits September 27, 2021 13:10

Do naming/documentation improvements

f0b4c1e

JAVA-4034

Improve docs and fix exception handling when getting source/connectio…

d467f2c

…n while writing JAVA-4034

Rename SourceOrConnectionInternalException to `ResourceSupplierInte…

97a1a56

…rnalException` JAVA-4034

Implement suggestions

dd9a50a

JAVA-4034

Add unit tests for LoopState/RetryState

eb7c698

JAVA-4034

Implement suggestions

070a2b5

JAVA-4034

Add the is prefix for methods returning a Boolean value.

43103db

JAVA-4034

Address concerns and improve docs

6f51a60

JAVA-4034

stIncMale force-pushed the JAVA-4034 branch from 165568a to 6f51a60 Compare September 27, 2021 19:11

stIncMale merged commit c11600d into mongodb:master Sep 27, 2021

stIncMale deleted the JAVA-4034 branch September 27, 2021 19:41

JAVA-4034 Refactor read/write retries to accommodate for both PoolClearedError and CSOT #782

JAVA-4034 Refactor read/write retries to accommodate for both PoolClearedError and CSOT #782

Uh oh!

Conversation

stIncMale commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stIncMale commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stIncMale Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

rozza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rozza Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

stIncMale Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stIncMale Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

jyemin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyemin left a comment

Choose a reason for hiding this comment

Uh oh!

jyemin Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

rozza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rozza Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rozza Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stIncMale commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JAVA-4034 Refactor read/write retries to accommodate for both `PoolClearedError` and CSOT #782

JAVA-4034 Refactor read/write retries to accommodate for both `PoolClearedError` and CSOT #782

stIncMale commented Sep 7, 2021 •

edited

Loading

stIncMale commented Sep 8, 2021 •

edited

Loading

jyemin left a comment •

edited

Loading

stIncMale commented Sep 27, 2021 •

edited

Loading