From e95ca0b2acf994256f3109aa3392e3b863d68b39 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Sat, 13 Apr 2024 20:47:15 -0600 Subject: [PATCH 01/19] Direct retries to another mongos if one is available Implemented the change and unit tests JAVA-4254, JAVA-5320 --- .../internal/connection/BaseCluster.java | 40 +++-- .../internal/connection/OperationContext.java | 86 +++++++++++ .../operation/AsyncOperationHelper.java | 13 +- .../operation/CommandOperationHelper.java | 19 ++- .../operation/MixedBulkWriteOperation.java | 13 +- .../operation/SyncOperationHelper.java | 12 +- .../ServerDeprioritizationTest.java | 145 ++++++++++++++++++ 7 files changed, 303 insertions(+), 25 deletions(-) create mode 100644 driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index 71526534c88..28279b02b2d 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -33,6 +33,7 @@ import com.mongodb.event.ClusterOpeningEvent; import com.mongodb.internal.VisibleForTesting; import com.mongodb.internal.async.SingleResultCallback; +import com.mongodb.internal.connection.OperationContext.ServerDeprioritization; import com.mongodb.internal.diagnostics.logging.Logger; import com.mongodb.internal.diagnostics.logging.Loggers; import com.mongodb.internal.logging.LogMessage; @@ -122,8 +123,9 @@ public ServerTuple selectServer(final ServerSelector serverSelector, final Opera CountDownLatch currentPhase = phase.get(); ClusterDescription curDescription = description; logServerSelectionStarted(clusterId, operationContext, serverSelector, curDescription); - ServerSelector compositeServerSelector = getCompositeServerSelector(serverSelector); - ServerTuple serverTuple = selectServer(compositeServerSelector, curDescription); + ServerDeprioritization serverDeprioritization = operationContext.getServerDeprioritization(); + ServerSelector completeServerSelector = getCompleteServerSelector(serverSelector, serverDeprioritization); + ServerTuple serverTuple = selectServer(completeServerSelector, curDescription); boolean selectionWaitingLogged = false; @@ -137,8 +139,10 @@ public ServerTuple selectServer(final ServerSelector serverSelector, final Opera } if (serverTuple != null) { + ServerAddress serverAddress = serverTuple.getServerDescription().getAddress(); logServerSelectionSucceeded( - clusterId, operationContext, serverTuple.getServerDescription().getAddress(), serverSelector, curDescription); + clusterId, operationContext, serverAddress, serverSelector, curDescription); + serverDeprioritization.updateCandidate(serverAddress, curDescription.getType()); return serverTuple; } @@ -163,7 +167,7 @@ public ServerTuple selectServer(final ServerSelector serverSelector, final Opera currentPhase = phase.get(); curDescription = description; - serverTuple = selectServer(compositeServerSelector, curDescription); + serverTuple = selectServer(completeServerSelector, curDescription); } } catch (InterruptedException e) { @@ -180,8 +184,9 @@ public void selectServerAsync(final ServerSelector serverSelector, final Operati ClusterDescription currentDescription = description; logServerSelectionStarted(clusterId, operationContext, serverSelector, currentDescription); - ServerSelectionRequest request = new ServerSelectionRequest(operationContext, serverSelector, getCompositeServerSelector(serverSelector), - getMaxWaitTimeNanos(), callback); + ServerSelectionRequest request = new ServerSelectionRequest(operationContext, serverSelector, + getCompleteServerSelector(serverSelector, operationContext.getServerDeprioritization()), + getMaxWaitTimeNanos(), callback); if (!handleServerSelectionRequest(request, currentPhase, currentDescription)) { notifyWaitQueueHandler(request); @@ -276,10 +281,12 @@ private boolean handleServerSelectionRequest(final ServerSelectionRequest reques return true; } - ServerTuple serverTuple = selectServer(request.compositeSelector, description); + ServerTuple serverTuple = selectServer(request.completeSelector, description); if (serverTuple != null) { - logServerSelectionSucceeded(clusterId, request.operationContext, serverTuple.getServerDescription().getAddress(), + ServerAddress serverAddress = serverTuple.getServerDescription().getAddress(); + logServerSelectionSucceeded(clusterId, request.operationContext, serverAddress, request.originalSelector, description); + request.operationContext.getServerDeprioritization().updateCandidate(serverAddress, description.getType()); request.onResult(serverTuple, null); return true; } @@ -343,14 +350,13 @@ private static List atMostNRandom(final ArrayList callback; @@ -407,13 +413,13 @@ private static final class ServerSelectionRequest { private CountDownLatch phase; ServerSelectionRequest(final OperationContext operationContext, - final ServerSelector serverSelector, final ServerSelector compositeSelector, + final ServerSelector serverSelector, final ServerSelector completeSelector, @Nullable final Long maxWaitTimeNanos, final SingleResultCallback callback) { this.operationContext = operationContext; this.originalSelector = serverSelector; - this.compositeSelector = compositeSelector; + this.completeSelector = completeSelector; this.maxWaitTimeNanos = maxWaitTimeNanos; this.callback = callback; } diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index 657aaccfab9..209fb076932 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -15,6 +15,17 @@ */ package com.mongodb.internal.connection; +import com.mongodb.MongoConnectionPoolClearedException; +import com.mongodb.ServerAddress; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ClusterType; +import com.mongodb.connection.ServerDescription; +import com.mongodb.lang.Nullable; +import com.mongodb.selector.ServerSelector; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; import java.util.concurrent.atomic.AtomicLong; /** @@ -23,12 +34,87 @@ public class OperationContext { private static final AtomicLong NEXT_ID = new AtomicLong(0); private final long id; + private final ServerDeprioritization serverDeprioritization; public OperationContext() { id = NEXT_ID.incrementAndGet(); + serverDeprioritization = new ServerDeprioritization(); } public long getId() { return id; } + + /** + * @return The same {@link ServerDeprioritization} if called on the same {@link OperationContext}. + */ + public ServerDeprioritization getServerDeprioritization() { + return serverDeprioritization; + } + + public static final class ServerDeprioritization { + @Nullable + private ServerAddress candidate; + private final Set deprioritized; + + private ServerDeprioritization() { + candidate = null; + deprioritized = new HashSet<>(); + } + + ServerSelector apply(final ServerSelector selector) { + return new DeprioritizingSelector(selector); + } + + void updateCandidate(final ServerAddress serverAddress, final ClusterType clusterType) { + candidate = isEnabled(clusterType) ? serverAddress : null; + } + + public void onAttemptFailure(final Throwable failure) { + if (!(failure instanceof MongoConnectionPoolClearedException)) { + deprioritized.add(candidate); + } + candidate = null; + } + + private static boolean isEnabled(final ClusterType clusterType) { + return clusterType == ClusterType.SHARDED; + } + + /** + * {@link ServerSelector} requires thread safety, but that is only because a user may specify + * {@link com.mongodb.connection.ClusterSettings.Builder#serverSelector(ServerSelector)}, + * which indeed may be used concurrently. {@link DeprioritizingSelector} does not need to be thread-safe. + */ + private final class DeprioritizingSelector implements ServerSelector { + private final ServerSelector wrapped; + + private DeprioritizingSelector(final ServerSelector wrapped) { + this.wrapped = wrapped; + } + + @Override + public List select(final ClusterDescription clusterDescription) { + if (isEnabled(clusterDescription.getType())) { + List filteredServerDescriptions = ClusterDescriptionHelper.getServersByPredicate( + clusterDescription, serverDescription -> !deprioritized.contains(serverDescription.getAddress())); + ClusterDescription filteredClusterDescription = new ClusterDescription( + clusterDescription.getConnectionMode(), + clusterDescription.getType(), + clusterDescription.getSrvResolutionException(), + filteredServerDescriptions, + clusterDescription.getClusterSettings(), + clusterDescription.getServerSettings()); + List result = wrapped.select(filteredClusterDescription); + if (result.isEmpty()) { + // fall back to select from all servers ignoring the deprioritized ones + result = wrapped.select(clusterDescription); + } + return result; + } else { + return wrapped.select(clusterDescription); + } + } + } + } } diff --git a/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java index 163521631d2..ab3a3afefd7 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java @@ -44,6 +44,7 @@ import java.util.Collections; import java.util.List; +import java.util.function.BiFunction; import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.internal.async.ErrorHandlingResultCallback.errorHandlingCallback; @@ -285,7 +286,11 @@ static void createReadCommandAndExecuteAsync( static AsyncCallbackSupplier decorateReadWithRetriesAsync(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier asyncReadFunction) { - return new RetryingAsyncCallbackSupplier<>(retryState, CommandOperationHelper::chooseRetryableReadException, + BiFunction onAttemptFailure = + (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> + CommandOperationHelper.onRetryableReadAttemptFailure( + operationContext, previouslyChosenException, mostRecentAttemptException); + return new RetryingAsyncCallbackSupplier<>(retryState, onAttemptFailure, CommandOperationHelper::shouldAttemptToRetryRead, callback -> { logRetryExecute(retryState, operationContext); asyncReadFunction.get(callback); @@ -294,7 +299,11 @@ static AsyncCallbackSupplier decorateReadWithRetriesAsync(final RetryStat static AsyncCallbackSupplier decorateWriteWithRetriesAsync(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier asyncWriteFunction) { - return new RetryingAsyncCallbackSupplier<>(retryState, CommandOperationHelper::chooseRetryableWriteException, + BiFunction onAttemptFailure = + (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> + CommandOperationHelper.onRetryableWriteAttemptFailure( + operationContext, previouslyChosenException, mostRecentAttemptException); + return new RetryingAsyncCallbackSupplier<>(retryState, onAttemptFailure, CommandOperationHelper::shouldAttemptToRetryWrite, callback -> { logRetryExecute(retryState, operationContext); asyncWriteFunction.get(callback); diff --git a/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java index fb1cc3c2da2..a9614d18788 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java @@ -51,8 +51,15 @@ interface CommandCreator { BsonDocument create(ServerDescription serverDescription, ConnectionDescription connectionDescription); } + static Throwable onRetryableReadAttemptFailure( + final OperationContext operationContext, + @Nullable final Throwable previouslyChosenException, + final Throwable mostRecentAttemptException) { + operationContext.getServerDeprioritization().onAttemptFailure(mostRecentAttemptException); + return chooseRetryableReadException(previouslyChosenException, mostRecentAttemptException); + } - static Throwable chooseRetryableReadException( + private static Throwable chooseRetryableReadException( @Nullable final Throwable previouslyChosenException, final Throwable mostRecentAttemptException) { assertFalse(mostRecentAttemptException instanceof ResourceSupplierInternalException); if (previouslyChosenException == null @@ -64,7 +71,15 @@ static Throwable chooseRetryableReadException( } } - static Throwable chooseRetryableWriteException( + static Throwable onRetryableWriteAttemptFailure( + final OperationContext operationContext, + @Nullable final Throwable previouslyChosenException, + final Throwable mostRecentAttemptException) { + operationContext.getServerDeprioritization().onAttemptFailure(mostRecentAttemptException); + return chooseRetryableWriteException(previouslyChosenException, mostRecentAttemptException); + } + + private static Throwable chooseRetryableWriteException( @Nullable final Throwable previouslyChosenException, final Throwable mostRecentAttemptException) { if (previouslyChosenException == null) { if (mostRecentAttemptException instanceof ResourceSupplierInternalException) { diff --git a/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java b/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java index fb54fb33994..2dac08e6302 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java +++ b/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java @@ -51,6 +51,7 @@ import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -140,7 +141,11 @@ public Boolean getRetryWrites() { private Supplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier writeFunction) { - return new RetryingSyncSupplier<>(retryState, CommandOperationHelper::chooseRetryableWriteException, + BiFunction onAttemptFailure = + (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> + CommandOperationHelper.onRetryableWriteAttemptFailure( + operationContext, previouslyChosenException, mostRecentAttemptException); + return new RetryingSyncSupplier<>(retryState, onAttemptFailure, this::shouldAttemptToRetryWrite, () -> { logRetryExecute(retryState, operationContext); return writeFunction.get(); @@ -149,7 +154,11 @@ private Supplier decorateWriteWithRetries(final RetryState retryState, fi private AsyncCallbackSupplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier writeFunction) { - return new RetryingAsyncCallbackSupplier<>(retryState, CommandOperationHelper::chooseRetryableWriteException, + BiFunction onAttemptFailure = + (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> + CommandOperationHelper.onRetryableWriteAttemptFailure( + operationContext, previouslyChosenException, mostRecentAttemptException); + return new RetryingAsyncCallbackSupplier<>(retryState, onAttemptFailure, this::shouldAttemptToRetryWrite, callback -> { logRetryExecute(retryState, operationContext); writeFunction.get(callback); diff --git a/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java index a10604bb717..4c9fdc424f3 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java @@ -274,7 +274,11 @@ static T createReadCommandAndExecute( static Supplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier writeFunction) { - return new RetryingSyncSupplier<>(retryState, CommandOperationHelper::chooseRetryableWriteException, + BiFunction onAttemptFailure = + (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> + CommandOperationHelper.onRetryableWriteAttemptFailure( + operationContext, previouslyChosenException, mostRecentAttemptException); + return new RetryingSyncSupplier<>(retryState, onAttemptFailure, CommandOperationHelper::shouldAttemptToRetryWrite, () -> { logRetryExecute(retryState, operationContext); return writeFunction.get(); @@ -283,7 +287,11 @@ static Supplier decorateWriteWithRetries(final RetryState retryState, static Supplier decorateReadWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier readFunction) { - return new RetryingSyncSupplier<>(retryState, CommandOperationHelper::chooseRetryableReadException, + BiFunction onAttemptFailure = + (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> + CommandOperationHelper.onRetryableReadAttemptFailure( + operationContext, previouslyChosenException, mostRecentAttemptException); + return new RetryingSyncSupplier<>(retryState, onAttemptFailure, CommandOperationHelper::shouldAttemptToRetryRead, () -> { logRetryExecute(retryState, operationContext); return readFunction.get(); diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java new file mode 100644 index 00000000000..9d59b676cae --- /dev/null +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java @@ -0,0 +1,145 @@ +/* + * 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.connection; + +import com.mongodb.MongoConnectionPoolClearedException; +import com.mongodb.ServerAddress; +import com.mongodb.connection.ClusterConnectionMode; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ClusterId; +import com.mongodb.connection.ClusterType; +import com.mongodb.connection.ServerConnectionState; +import com.mongodb.connection.ServerDescription; +import com.mongodb.connection.ServerId; +import com.mongodb.internal.connection.OperationContext.ServerDeprioritization; +import com.mongodb.internal.selector.ServerAddressSelector; +import com.mongodb.selector.ServerSelector; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import java.util.List; + +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static java.util.Collections.unmodifiableList; +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.params.provider.EnumSource.Mode.EXCLUDE; + +final class ServerDeprioritizationTest { + private static final ServerDescription SERVER_A = serverDescription("a"); + private static final ServerDescription SERVER_B = serverDescription("b"); + private static final ServerDescription SERVER_C = serverDescription("c"); + private static final List ALL_SERVERS = unmodifiableList(asList(SERVER_A, SERVER_B, SERVER_C)); + private static final ClusterDescription REPLICA_SET = clusterDescription(ClusterType.REPLICA_SET); + private static final ClusterDescription SHARDED_CLUSTER = clusterDescription(ClusterType.SHARDED); + private static final ServerSelector ALL_SELECTOR = new AllServerSelector(); + + private ServerDeprioritization serverDeprioritization; + + @BeforeEach + void beforeEach() { + serverDeprioritization = new OperationContext().getServerDeprioritization(); + } + + @Test + void applyNoneDeprioritized() { + assertAll( + () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)), + () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(REPLICA_SET)) + ); + } + + @Test + void applySomeDeprioritized() { + deprioritize(SERVER_B); + assertAll( + () -> assertEquals(asList(SERVER_A, SERVER_C), serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)), + () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(REPLICA_SET)) + ); + } + + @Test + void applyFallsBack() { + assertAll( + () -> { + deprioritize(SERVER_A); + deprioritize(SERVER_B); + deprioritize(SERVER_C); + assertAll( + () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)), + () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(REPLICA_SET)) + ); + }, + () -> { + ServerSelector serverBSelector = new ServerAddressSelector(SERVER_B.getAddress()); + deprioritize(SERVER_B); + assertAll( + () -> assertEquals(singletonList(SERVER_B), + serverDeprioritization.apply(serverBSelector).select(SHARDED_CLUSTER)), + () -> assertEquals(singletonList(SERVER_B), serverDeprioritization.apply(serverBSelector).select(REPLICA_SET)) + ); + } + ); + } + + @ParameterizedTest + @EnumSource(value = ClusterType.class, mode = EXCLUDE, names = {"SHARDED"}) + void updateCandidateIgnoresIfNotShardedCluster(final ClusterType clusterType) { + serverDeprioritization.updateCandidate(SERVER_A.getAddress(), clusterType); + serverDeprioritization.onAttemptFailure(new RuntimeException()); + assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)); + } + + @Test + void onAttemptFailureIgnoresIfPoolClearedException() { + serverDeprioritization.updateCandidate(SERVER_A.getAddress(), ClusterType.SHARDED); + serverDeprioritization.onAttemptFailure( + new MongoConnectionPoolClearedException(new ServerId(new ClusterId(), new ServerAddress()), null)); + assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)); + } + + private void deprioritize(final ServerDescription... serverDescriptions) { + for (ServerDescription serverDescription : serverDescriptions) { + serverDeprioritization.updateCandidate(serverDescription.getAddress(), ClusterType.SHARDED); + serverDeprioritization.onAttemptFailure(new RuntimeException()); + } + } + + private static ServerDescription serverDescription(final String host) { + return ServerDescription.builder() + .state(ServerConnectionState.CONNECTED) + .ok(true) + .address(new ServerAddress(host)) + .build(); + } + + private static ClusterDescription clusterDescription(final ClusterType clusterType) { + return new ClusterDescription(ClusterConnectionMode.MULTIPLE, clusterType, asList(SERVER_A, SERVER_B, SERVER_C)); + } + + private static final class AllServerSelector implements ServerSelector { + AllServerSelector() { + } + + @Override + public List select(final ClusterDescription clusterDescription) { + return clusterDescription.getServerDescriptions(); + } + } +} From d1c1ed2c9cfe3da0ba0b30ab29c144c3b043ef02 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Tue, 16 Apr 2024 19:17:11 -0600 Subject: [PATCH 02/19] Implement specification prose tests JAVA-4254, JAVA-5320 --- .../client/RetryableReadsProseTest.java | 38 ++++- .../client/RetryableWritesProseTest.java | 26 +++- .../client/RetryableReadsProseTest.java | 35 ++++- .../client/RetryableWritesProseTest.java | 147 +++++++++++++++++- 4 files changed, 236 insertions(+), 10 deletions(-) diff --git a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableReadsProseTest.java b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableReadsProseTest.java index b29d2df8241..21ef7698248 100644 --- a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableReadsProseTest.java +++ b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableReadsProseTest.java @@ -16,8 +16,10 @@ package com.mongodb.reactivestreams.client; +import com.mongodb.client.MongoCursor; import com.mongodb.client.RetryableWritesProseTest; import com.mongodb.reactivestreams.client.syncadapter.SyncMongoClient; +import org.bson.Document; import org.junit.jupiter.api.Test; import java.util.concurrent.ExecutionException; @@ -29,16 +31,48 @@ * See * Retryable Reads Tests. */ -public class RetryableReadsProseTest { +final class RetryableReadsProseTest { /** * See * * PoolClearedError Retryability Test. */ @Test - public void poolClearedExceptionMustBeRetryable() throws InterruptedException, ExecutionException, TimeoutException { + void poolClearedExceptionMustBeRetryable() throws InterruptedException, ExecutionException, TimeoutException { RetryableWritesProseTest.poolClearedExceptionMustBeRetryable( mongoClientSettings -> new SyncMongoClient(MongoClients.create(mongoClientSettings)), mongoCollection -> mongoCollection.find(eq(0)).iterator().hasNext(), "find", false); } + + /** + * See + * + * Retryable Reads Are Retried on a Different mongos When One is Available. + */ + @Test + void retriesOnDifferentMongosWhenAvailable() { + RetryableWritesProseTest.retriesOnDifferentMongosWhenAvailable( + mongoClientSettings -> new SyncMongoClient(MongoClients.create(mongoClientSettings)), + mongoCollection -> { + try (MongoCursor cursor = mongoCollection.find().iterator()) { + return cursor.hasNext(); + } + }, "find", false); + } + + /** + * See + * + * Retryable Reads Are Retried on the Same mongos When No Others are Available. + */ + @Test + void retriesOnSameMongosWhenAnotherNotAvailable() { + RetryableWritesProseTest.retriesOnSameMongosWhenAnotherNotAvailable( + mongoClientSettings -> new SyncMongoClient(MongoClients.create(mongoClientSettings)), + mongoCollection -> { + try (MongoCursor cursor = mongoCollection.find().iterator()) { + return cursor.hasNext(); + } + }, "find", false); + } } diff --git a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableWritesProseTest.java b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableWritesProseTest.java index eb2b73e0c7e..fc018a06068 100644 --- a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableWritesProseTest.java +++ b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/RetryableWritesProseTest.java @@ -57,7 +57,7 @@ public void setUp() { @Test public void testRetryWritesWithInsertOneAgainstMMAPv1RaisesError() { - assumeTrue(canRunTests()); + assumeTrue(canRunMmapv1Tests()); boolean exceptionFound = false; try { @@ -74,7 +74,7 @@ public void testRetryWritesWithInsertOneAgainstMMAPv1RaisesError() { @Test public void testRetryWritesWithFindOneAndDeleteAgainstMMAPv1RaisesError() { - assumeTrue(canRunTests()); + assumeTrue(canRunMmapv1Tests()); boolean exceptionFound = false; try { @@ -108,7 +108,27 @@ public void originalErrorMustBePropagatedIfNoWritesPerformed() throws Interrupte mongoClientSettings -> new SyncMongoClient(MongoClients.create(mongoClientSettings))); } - private boolean canRunTests() { + /** + * Prose test #4. + */ + @Test + public void retriesOnDifferentMongosWhenAvailable() { + com.mongodb.client.RetryableWritesProseTest.retriesOnDifferentMongosWhenAvailable( + mongoClientSettings -> new SyncMongoClient(MongoClients.create(mongoClientSettings)), + mongoCollection -> mongoCollection.insertOne(new Document()), "insert", true); + } + + /** + * Prose test #5. + */ + @Test + public void retriesOnSameMongosWhenAnotherNotAvailable() { + com.mongodb.client.RetryableWritesProseTest.retriesOnSameMongosWhenAnotherNotAvailable( + mongoClientSettings -> new SyncMongoClient(MongoClients.create(mongoClientSettings)), + mongoCollection -> mongoCollection.insertOne(new Document()), "insert", true); + } + + private boolean canRunMmapv1Tests() { Document storageEngine = (Document) getServerStatus().get("storageEngine"); return ((isSharded() || isDiscoverableReplicaSet()) diff --git a/driver-sync/src/test/functional/com/mongodb/client/RetryableReadsProseTest.java b/driver-sync/src/test/functional/com/mongodb/client/RetryableReadsProseTest.java index cb1f40bfe81..ccf18aad5b9 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/RetryableReadsProseTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/RetryableReadsProseTest.java @@ -16,6 +16,7 @@ package com.mongodb.client; +import org.bson.Document; import org.junit.jupiter.api.Test; import java.util.concurrent.ExecutionException; @@ -27,15 +28,45 @@ * See * Retryable Reads Tests. */ -public class RetryableReadsProseTest { +final class RetryableReadsProseTest { /** * See * * PoolClearedError Retryability Test. */ @Test - public void poolClearedExceptionMustBeRetryable() throws InterruptedException, ExecutionException, TimeoutException { + void poolClearedExceptionMustBeRetryable() throws InterruptedException, ExecutionException, TimeoutException { RetryableWritesProseTest.poolClearedExceptionMustBeRetryable(MongoClients::create, mongoCollection -> mongoCollection.find(eq(0)).iterator().hasNext(), "find", false); } + + /** + * See + * + * Retryable Reads Are Retried on a Different mongos When One is Available. + */ + @Test + void retriesOnDifferentMongosWhenAvailable() { + RetryableWritesProseTest.retriesOnDifferentMongosWhenAvailable(MongoClients::create, + mongoCollection -> { + try (MongoCursor cursor = mongoCollection.find().iterator()) { + return cursor.hasNext(); + } + }, "find", false); + } + + /** + * See + * + * Retryable Reads Are Retried on the Same mongos When No Others are Available. + */ + @Test + void retriesOnSameMongosWhenAnotherNotAvailable() { + RetryableWritesProseTest.retriesOnSameMongosWhenAnotherNotAvailable(MongoClients::create, + mongoCollection -> { + try (MongoCursor cursor = mongoCollection.find().iterator()) { + return cursor.hasNext(); + } + }, "find", false); + } } diff --git a/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java b/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java index e449fc628af..f664f35f947 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java @@ -16,6 +16,7 @@ package com.mongodb.client; +import com.mongodb.ConnectionString; import com.mongodb.Function; import com.mongodb.MongoClientException; import com.mongodb.MongoClientSettings; @@ -23,11 +24,16 @@ import com.mongodb.MongoWriteConcernException; import com.mongodb.ServerAddress; import com.mongodb.assertions.Assertions; +import com.mongodb.connection.ClusterConnectionMode; +import com.mongodb.connection.ConnectionDescription; +import com.mongodb.event.CommandEvent; +import com.mongodb.event.CommandFailedEvent; import com.mongodb.event.CommandListener; import com.mongodb.event.CommandSucceededEvent; import com.mongodb.event.ConnectionCheckOutFailedEvent; import com.mongodb.event.ConnectionCheckedOutEvent; import com.mongodb.event.ConnectionPoolClearedEvent; +import com.mongodb.internal.connection.ServerAddressHelper; import com.mongodb.internal.connection.TestCommandListener; import com.mongodb.internal.connection.TestConnectionPoolListener; import org.bson.BsonArray; @@ -39,6 +45,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.util.HashSet; +import java.util.List; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -50,6 +59,8 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static com.mongodb.ClusterFixture.getConnectionString; +import static com.mongodb.ClusterFixture.getMultiMongosConnectionString; import static com.mongodb.ClusterFixture.getServerStatus; import static com.mongodb.ClusterFixture.isDiscoverableReplicaSet; import static com.mongodb.ClusterFixture.isServerlessTest; @@ -59,10 +70,13 @@ import static com.mongodb.ClusterFixture.serverVersionLessThan; import static com.mongodb.client.Fixture.getDefaultDatabaseName; import static com.mongodb.client.Fixture.getMongoClientSettingsBuilder; +import static com.mongodb.client.Fixture.getMultiMongosMongoClientSettingsBuilder; import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.concurrent.TimeUnit.SECONDS; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeFalse; @@ -82,7 +96,7 @@ public void setUp() { @Test public void testRetryWritesWithInsertOneAgainstMMAPv1RaisesError() { - assumeTrue(canRunTests()); + assumeTrue(canRunMmapv1Tests()); boolean exceptionFound = false; try { @@ -99,7 +113,7 @@ public void testRetryWritesWithInsertOneAgainstMMAPv1RaisesError() { @Test public void testRetryWritesWithFindOneAndDeleteAgainstMMAPv1RaisesError() { - assumeTrue(canRunTests()); + assumeTrue(canRunMmapv1Tests()); boolean exceptionFound = false; try { @@ -256,7 +270,134 @@ public void commandSucceeded(final CommandSucceededEvent event) { } } - private boolean canRunTests() { + /** + * Prose test #4. + */ + @Test + public void retriesOnDifferentMongosWhenAvailable() { + retriesOnDifferentMongosWhenAvailable(MongoClients::create, + mongoCollection -> mongoCollection.insertOne(new Document()), "insert", true); + } + + @SuppressWarnings("try") + public static void retriesOnDifferentMongosWhenAvailable( + final Function clientCreator, + final Function, R> operation, final String operationName, final boolean write) { + if (write) { + assumeTrue(serverVersionAtLeast(4, 4)); + } else { + assumeTrue(serverVersionAtLeast(4, 2)); + } + assumeTrue(isSharded()); + ConnectionString connectionString = getMultiMongosConnectionString(); + assumeTrue(connectionString != null); + ServerAddress s0Address = ServerAddressHelper.createServerAddress(connectionString.getHosts().get(0)); + ServerAddress s1Address = ServerAddressHelper.createServerAddress(connectionString.getHosts().get(1)); + BsonDocument failPointDocument = BsonDocument.parse( + "{\n" + + " configureFailPoint: \"failCommand\",\n" + + " mode: { times: 1 },\n" + + " data: {\n" + + " failCommands: [\"" + operationName + "\"],\n" + + (write + ? " errorLabels: [\"RetryableWriteError\"]," : "") + + " errorCode: 6\n" + + " }\n" + + "}\n"); + TestCommandListener commandListener = new TestCommandListener(singletonList("commandFailedEvent"), emptyList()); + try (FailPoint s0FailPoint = FailPoint.enable(failPointDocument, s0Address); + FailPoint s1FailPoint = FailPoint.enable(failPointDocument, s1Address); + MongoClient client = clientCreator.apply(getMultiMongosMongoClientSettingsBuilder() + .retryReads(true) + .retryWrites(true) + .addCommandListener(commandListener) + // explicitly specify only s0 and s1, in case `getMultiMongosMongoClientSettingsBuilder` has more + .applyToClusterSettings(builder -> builder.hosts(asList(s0Address, s1Address))) + .build())) { + MongoCollection collection = client.getDatabase(getDefaultDatabaseName()) + .getCollection("retriesOnDifferentMongosWhenAvailable"); + collection.drop(); + commandListener.reset(); + assertThrows(RuntimeException.class, () -> operation.apply(collection)); + List failedCommandEvents = commandListener.getEvents(); + assertEquals(2, failedCommandEvents.size(), failedCommandEvents::toString); + List unexpectedCommandNames = failedCommandEvents.stream() + .map(CommandEvent::getCommandName) + .filter(commandName -> !commandName.equals(operationName)) + .collect(Collectors.toList()); + assertTrue(unexpectedCommandNames.isEmpty(), unexpectedCommandNames::toString); + Set failedServerAddresses = failedCommandEvents.stream() + .map(CommandEvent::getConnectionDescription) + .map(ConnectionDescription::getServerAddress) + .collect(Collectors.toSet()); + assertEquals(new HashSet<>(asList(s0Address, s1Address)), failedServerAddresses); + } + } + + /** + * Prose test #5. + */ + @Test + public void retriesOnSameMongosWhenAnotherNotAvailable() { + retriesOnSameMongosWhenAnotherNotAvailable(MongoClients::create, + mongoCollection -> mongoCollection.insertOne(new Document()), "insert", true); + } + + @SuppressWarnings("try") + public static void retriesOnSameMongosWhenAnotherNotAvailable( + final Function clientCreator, + final Function, R> operation, final String operationName, final boolean write) { + if (write) { + assumeTrue(serverVersionAtLeast(4, 4)); + } else { + assumeTrue(serverVersionAtLeast(4, 2)); + } + assumeTrue(isSharded()); + ConnectionString connectionString = getConnectionString(); + ServerAddress s0Address = ServerAddressHelper.createServerAddress(connectionString.getHosts().get(0)); + BsonDocument failPointDocument = BsonDocument.parse( + "{\n" + + " configureFailPoint: \"failCommand\",\n" + + " mode: { times: 1 },\n" + + " data: {\n" + + " failCommands: [\"" + operationName + "\"],\n" + + (write + ? " errorLabels: [\"RetryableWriteError\"]," : "") + + " errorCode: 6\n" + + " }\n" + + "}\n"); + TestCommandListener commandListener = new TestCommandListener( + asList("commandFailedEvent", "commandSucceededEvent"), emptyList()); + try (FailPoint s0FailPoint = FailPoint.enable(failPointDocument, s0Address); + MongoClient client = clientCreator.apply(getMongoClientSettingsBuilder() + .retryReads(true) + .retryWrites(true) + .addCommandListener(commandListener) + // explicitly specify only s0, in case `getMongoClientSettingsBuilder` has more + .applyToClusterSettings(builder -> builder + .hosts(singletonList(s0Address)) + .mode(ClusterConnectionMode.MULTIPLE)) + .build())) { + MongoCollection collection = client.getDatabase(getDefaultDatabaseName()) + .getCollection("retriesOnSameMongosWhenAnotherNotAvailable"); + collection.drop(); + commandListener.reset(); + operation.apply(collection); + List commandEvents = commandListener.getEvents(); + assertEquals(2, commandEvents.size(), commandEvents::toString); + List unexpectedCommandNames = commandEvents.stream() + .map(CommandEvent::getCommandName) + .filter(commandName -> !commandName.equals(operationName)) + .collect(Collectors.toList()); + assertTrue(unexpectedCommandNames.isEmpty(), unexpectedCommandNames::toString); + assertInstanceOf(CommandFailedEvent.class, commandEvents.get(0), commandEvents::toString); + assertEquals(s0Address, commandEvents.get(0).getConnectionDescription().getServerAddress(), commandEvents::toString); + assertInstanceOf(CommandSucceededEvent.class, commandEvents.get(1), commandEvents::toString); + assertEquals(s0Address, commandEvents.get(1).getConnectionDescription().getServerAddress(), commandEvents::toString); + } + } + + private boolean canRunMmapv1Tests() { Document storageEngine = (Document) getServerStatus().get("storageEngine"); return ((isSharded() || isDiscoverableReplicaSet()) From 0875e5dbc58b7b9d2d266b9269c146ca59cda33d Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Tue, 16 Apr 2024 21:51:30 -0600 Subject: [PATCH 03/19] Fix a typo JAVA-4254, JAVA-5320 --- .../main/com/mongodb/internal/connection/OperationContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index 209fb076932..5672229801f 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -107,7 +107,7 @@ public List select(final ClusterDescription clusterDescriptio clusterDescription.getServerSettings()); List result = wrapped.select(filteredClusterDescription); if (result.isEmpty()) { - // fall back to select from all servers ignoring the deprioritized ones + // fall back to selecting from all servers ignoring the deprioritized ones result = wrapped.select(clusterDescription); } return result; From 6fe3db1862a90f626da37e055103414488ca5d46 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Thu, 18 Apr 2024 11:08:22 -0600 Subject: [PATCH 04/19] Expect `MongoServerException` instead of just `RuntimeException` JAVA-4254, JAVA-5320 --- .../com/mongodb/client/RetryableWritesProseTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java b/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java index f664f35f947..ce4a642805b 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java @@ -21,6 +21,7 @@ import com.mongodb.MongoClientException; import com.mongodb.MongoClientSettings; import com.mongodb.MongoException; +import com.mongodb.MongoServerException; import com.mongodb.MongoWriteConcernException; import com.mongodb.ServerAddress; import com.mongodb.assertions.Assertions; @@ -318,7 +319,7 @@ public static void retriesOnDifferentMongosWhenAvailable( .getCollection("retriesOnDifferentMongosWhenAvailable"); collection.drop(); commandListener.reset(); - assertThrows(RuntimeException.class, () -> operation.apply(collection)); + assertThrows(MongoServerException.class, () -> operation.apply(collection)); List failedCommandEvents = commandListener.getEvents(); assertEquals(2, failedCommandEvents.size(), failedCommandEvents::toString); List unexpectedCommandNames = failedCommandEvents.stream() From 7cba88b2fab9027d7eab35ed8dfdaddaf4d5a868 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Mon, 29 Apr 2024 19:48:34 -0600 Subject: [PATCH 05/19] Fix `ServerDeprioritization.onAttemptFailure` JAVA-4254 --- .../com/mongodb/internal/connection/OperationContext.java | 2 +- .../internal/connection/ServerDeprioritizationTest.java | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index 5672229801f..c2f4e653fed 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -71,7 +71,7 @@ void updateCandidate(final ServerAddress serverAddress, final ClusterType cluste } public void onAttemptFailure(final Throwable failure) { - if (!(failure instanceof MongoConnectionPoolClearedException)) { + if (candidate != null && !(failure instanceof MongoConnectionPoolClearedException)) { deprioritized.add(candidate); } candidate = null; diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java index 9d59b676cae..88702c1e1be 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java @@ -38,6 +38,7 @@ import static java.util.Collections.singletonList; import static java.util.Collections.unmodifiableList; import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.params.provider.EnumSource.Mode.EXCLUDE; @@ -114,6 +115,11 @@ void onAttemptFailureIgnoresIfPoolClearedException() { assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)); } + @Test + void onAttemptFailureDoesNotThrowIfNoCandidate() { + assertDoesNotThrow(() -> serverDeprioritization.onAttemptFailure(new RuntimeException())); + } + private void deprioritize(final ServerDescription... serverDescriptions) { for (ServerDescription serverDescription : serverDescriptions) { serverDeprioritization.updateCandidate(serverDescription.getAddress(), ClusterType.SHARDED); From e4ffab42ff04e943bd2b9fcdf2689ec3d499b7ad Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Mon, 29 Apr 2024 23:37:42 -0600 Subject: [PATCH 06/19] Replace BiFunction with BinaryOperator JAVA-4254 --- .../mongodb/internal/operation/AsyncOperationHelper.java | 6 +++--- .../mongodb/internal/operation/MixedBulkWriteOperation.java | 6 +++--- .../com/mongodb/internal/operation/SyncOperationHelper.java | 5 +++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java index ab3a3afefd7..1a3d62ec792 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java @@ -44,7 +44,7 @@ import java.util.Collections; import java.util.List; -import java.util.function.BiFunction; +import java.util.function.BinaryOperator; import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.internal.async.ErrorHandlingResultCallback.errorHandlingCallback; @@ -286,7 +286,7 @@ static void createReadCommandAndExecuteAsync( static AsyncCallbackSupplier decorateReadWithRetriesAsync(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier asyncReadFunction) { - BiFunction onAttemptFailure = + BinaryOperator onAttemptFailure = (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> CommandOperationHelper.onRetryableReadAttemptFailure( operationContext, previouslyChosenException, mostRecentAttemptException); @@ -299,7 +299,7 @@ static AsyncCallbackSupplier decorateReadWithRetriesAsync(final RetryStat static AsyncCallbackSupplier decorateWriteWithRetriesAsync(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier asyncWriteFunction) { - BiFunction onAttemptFailure = + BinaryOperator onAttemptFailure = (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> CommandOperationHelper.onRetryableWriteAttemptFailure( operationContext, previouslyChosenException, mostRecentAttemptException); diff --git a/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java b/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java index 2dac08e6302..ff30807b2b8 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java +++ b/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java @@ -51,7 +51,7 @@ import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.function.BiFunction; +import java.util.function.BinaryOperator; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -141,7 +141,7 @@ public Boolean getRetryWrites() { private Supplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier writeFunction) { - BiFunction onAttemptFailure = + BinaryOperator onAttemptFailure = (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> CommandOperationHelper.onRetryableWriteAttemptFailure( operationContext, previouslyChosenException, mostRecentAttemptException); @@ -154,7 +154,7 @@ private Supplier decorateWriteWithRetries(final RetryState retryState, fi private AsyncCallbackSupplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier writeFunction) { - BiFunction onAttemptFailure = + BinaryOperator onAttemptFailure = (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> CommandOperationHelper.onRetryableWriteAttemptFailure( operationContext, previouslyChosenException, mostRecentAttemptException); diff --git a/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java index 4c9fdc424f3..6f548f4b4d2 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java @@ -41,6 +41,7 @@ import org.bson.codecs.Decoder; import java.util.function.BiFunction; +import java.util.function.BinaryOperator; import java.util.function.Function; import java.util.function.Supplier; @@ -274,7 +275,7 @@ static T createReadCommandAndExecute( static Supplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier writeFunction) { - BiFunction onAttemptFailure = + BinaryOperator onAttemptFailure = (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> CommandOperationHelper.onRetryableWriteAttemptFailure( operationContext, previouslyChosenException, mostRecentAttemptException); @@ -287,7 +288,7 @@ static Supplier decorateWriteWithRetries(final RetryState retryState, static Supplier decorateReadWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier readFunction) { - BiFunction onAttemptFailure = + BinaryOperator onAttemptFailure = (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> CommandOperationHelper.onRetryableReadAttemptFailure( operationContext, previouslyChosenException, mostRecentAttemptException); From 29ebd766acaa2066911520f51a11ea79b0ea3e36 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Wed, 1 May 2024 20:19:57 -0600 Subject: [PATCH 07/19] Update driver-core/src/main/com/mongodb/internal/connection/OperationContext.java Let's put various checks (validation, preconditions...) at the top of methods, with the operation at the bottom. Co-authored-by: Maxim Katcharov --- .../com/mongodb/internal/connection/OperationContext.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index c2f4e653fed..b7289f3b7a4 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -71,10 +71,11 @@ void updateCandidate(final ServerAddress serverAddress, final ClusterType cluste } public void onAttemptFailure(final Throwable failure) { - if (candidate != null && !(failure instanceof MongoConnectionPoolClearedException)) { - deprioritized.add(candidate); + if (candidate == null || failure instanceof MongoConnectionPoolClearedException) { + candidate = null; + return; } - candidate = null; + deprioritized.add(candidate); } private static boolean isEnabled(final ClusterType clusterType) { From 6574e241434f8c8946984ade559695e2aa9f29b3 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Thu, 2 May 2024 17:56:19 -0600 Subject: [PATCH 08/19] Trivial code improvements JAVA-4254 --- .../internal/connection/BaseCluster.java | 15 +++++++----- .../internal/connection/OperationContext.java | 23 +++++++++++-------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index 28279b02b2d..8bebadb8dba 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -49,6 +49,8 @@ import java.util.Deque; import java.util.Iterator; import java.util.List; +import java.util.Objects; +import java.util.stream.Stream; import java.util.concurrent.ConcurrentLinkedDeque; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ThreadLocalRandom; @@ -82,6 +84,7 @@ import static java.util.Comparator.comparingInt; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.stream.Collectors.toList; abstract class BaseCluster implements Cluster { private static final Logger LOGGER = Loggers.getLogger("cluster"); @@ -351,12 +354,12 @@ private static List atMostNRandom(final ArrayList selectors = Stream.of( + serverSelector, + settings.getServerSelector(), + new LatencyMinimizingServerSelector(settings.getLocalThreshold(MILLISECONDS), MILLISECONDS) + ).filter(Objects::nonNull).collect(toList()); + return serverDeprioritization.apply(new CompositeServerSelector(selectors)); } protected ClusterableServer createServer(final ServerAddress serverAddress) { diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index b7289f3b7a4..03fb6ae71e2 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -97,16 +97,10 @@ private DeprioritizingSelector(final ServerSelector wrapped) { @Override public List select(final ClusterDescription clusterDescription) { if (isEnabled(clusterDescription.getType())) { - List filteredServerDescriptions = ClusterDescriptionHelper.getServersByPredicate( + List nonDeprioritizedServerDescriptions = ClusterDescriptionHelper.getServersByPredicate( clusterDescription, serverDescription -> !deprioritized.contains(serverDescription.getAddress())); - ClusterDescription filteredClusterDescription = new ClusterDescription( - clusterDescription.getConnectionMode(), - clusterDescription.getType(), - clusterDescription.getSrvResolutionException(), - filteredServerDescriptions, - clusterDescription.getClusterSettings(), - clusterDescription.getServerSettings()); - List result = wrapped.select(filteredClusterDescription); + List result = wrapped.select( + copyWithServerDescriptions(clusterDescription, nonDeprioritizedServerDescriptions)); if (result.isEmpty()) { // fall back to selecting from all servers ignoring the deprioritized ones result = wrapped.select(clusterDescription); @@ -116,6 +110,17 @@ public List select(final ClusterDescription clusterDescriptio return wrapped.select(clusterDescription); } } + + private ClusterDescription copyWithServerDescriptions( + final ClusterDescription clusterDescription, final List serverDescriptions) { + return new ClusterDescription( + clusterDescription.getConnectionMode(), + clusterDescription.getType(), + clusterDescription.getSrvResolutionException(), + serverDescriptions, + clusterDescription.getClusterSettings(), + clusterDescription.getServerSettings()); + } } } } From 171d67ed767a65156a768c9667488b13d11049eb Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Thu, 2 May 2024 20:18:10 -0600 Subject: [PATCH 09/19] Update the docs of the internal API for retries to reflect support of side effects JAVA-4254 --- .../internal/async/function/RetryState.java | 62 ++++++++++--------- .../RetryingAsyncCallbackSupplier.java | 35 ++++++----- .../async/function/RetryingSyncSupplier.java | 12 ++-- 3 files changed, 59 insertions(+), 50 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/async/function/RetryState.java b/driver-core/src/main/com/mongodb/internal/async/function/RetryState.java index ba4da185d79..89329f16a24 100644 --- a/driver-core/src/main/com/mongodb/internal/async/function/RetryState.java +++ b/driver-core/src/main/com/mongodb/internal/async/function/RetryState.java @@ -78,24 +78,25 @@ public RetryState() { * which is usually synchronous code. * * @param attemptException The exception produced by the most recent attempt. - * It is passed to the {@code retryPredicate} and to the {@code exceptionTransformer}. - * @param exceptionTransformer A function that chooses which exception to preserve as a prospective failed result of the associated - * retryable activity and may also transform or mutate the exceptions. - * The choice is between + * It is passed to the {@code retryPredicate} and to the {@code onAttemptFailureOperator}. + * @param onAttemptFailureOperator The action that is called once per failed attempt before (in the happens-before order) the + * {@code retryPredicate}, regardless of whether the {@code retryPredicate} is called. + * This action is allowed to have side effects. + *

+ * It also has to choose which exception to preserve as a prospective failed result of the associated retryable activity. + * The {@code onAttemptFailureOperator} may mutate its arguments, choose from the arguments, or return a different exception, + * but it must return a {@code @}{@link NonNull} value. + * The choice is between

*
    *
  • the previously chosen exception or {@code null} if none has been chosen - * (the first argument of the {@code exceptionTransformer})
    • - *
  • and the exception from the most recent attempt (the second argument of the {@code exceptionTransformer}).
    • + * (the first argument of the {@code onAttemptFailureOperator}) + *
  • and the exception from the most recent attempt (the second argument of the {@code onAttemptFailureOperator}).
    • *
- * The {@code exceptionTransformer} may either choose from its arguments, or return a different exception, a.k.a. transform, - * but it must return a {@code @}{@link NonNull} value. - * The {@code exceptionTransformer} is called once before (in the happens-before order) the {@code retryPredicate}, - * regardless of whether the {@code retryPredicate} is called. The result of the {@code exceptionTransformer} does not affect - * what exception is passed to the {@code retryPredicate}. + * The result of the {@code onAttemptFailureOperator} does not affect the exception passed to the {@code retryPredicate}. * @param retryPredicate {@code true} iff another attempt needs to be made. The {@code retryPredicate} is called not more than once * per attempt and only if all the following is true: *
    - *
  • {@code exceptionTransformer} completed normally;
    • + *
  • {@code onAttemptFailureOperator} completed normally;
    • *
  • the most recent attempt is not the {@linkplain #isLastAttempt() last} one.
    • *
* The {@code retryPredicate} accepts this {@link RetryState} and the exception from the most recent attempt, @@ -103,7 +104,7 @@ public RetryState() { * after (in the happens-before order) testing the {@code retryPredicate}, and only if the predicate completes normally. * @throws RuntimeException Iff any of the following is true: *
    - *
  • the {@code exceptionTransformer} completed abruptly;
    • + *
  • the {@code onAttemptFailureOperator} completed abruptly;
    • *
  • the most recent attempt is the {@linkplain #isLastAttempt() last} one;
    • *
  • the {@code retryPredicate} completed abruptly;
    • *
  • the {@code retryPredicate} is {@code false}.
    • @@ -112,10 +113,10 @@ public RetryState() { * i.e., the caller must not do any more attempts. * @see #advanceOrThrow(Throwable, BinaryOperator, BiPredicate) */ - void advanceOrThrow(final RuntimeException attemptException, final BinaryOperator exceptionTransformer, + void advanceOrThrow(final RuntimeException attemptException, final BinaryOperator onAttemptFailureOperator, final BiPredicate retryPredicate) throws RuntimeException { try { - doAdvanceOrThrow(attemptException, exceptionTransformer, retryPredicate, true); + doAdvanceOrThrow(attemptException, onAttemptFailureOperator, retryPredicate, true); } catch (RuntimeException | Error unchecked) { throw unchecked; } catch (Throwable checked) { @@ -129,18 +130,19 @@ void advanceOrThrow(final RuntimeException attemptException, final BinaryOperato * * @see #advanceOrThrow(RuntimeException, BinaryOperator, BiPredicate) */ - void advanceOrThrow(final Throwable attemptException, final BinaryOperator exceptionTransformer, + void advanceOrThrow(final Throwable attemptException, final BinaryOperator onAttemptFailureOperator, final BiPredicate retryPredicate) throws Throwable { - doAdvanceOrThrow(attemptException, exceptionTransformer, retryPredicate, false); + doAdvanceOrThrow(attemptException, onAttemptFailureOperator, retryPredicate, false); } /** * @param onlyRuntimeExceptions {@code true} iff the method must expect {@link #exception} and {@code attemptException} to be * {@link RuntimeException}s and must not explicitly handle other {@link Throwable} types, of which only {@link Error} is possible * as {@link RetryState} does not have any source of {@link Exception}s. + * @param onAttemptFailureOperator See {@link #advanceOrThrow(RuntimeException, BinaryOperator, BiPredicate)}. */ private void doAdvanceOrThrow(final Throwable attemptException, - final BinaryOperator exceptionTransformer, + final BinaryOperator onAttemptFailureOperator, final BiPredicate retryPredicate, final boolean onlyRuntimeExceptions) throws Throwable { assertTrue(attempt() < attempts); @@ -149,7 +151,7 @@ private void doAdvanceOrThrow(final Throwable attemptException, assertTrue(isRuntime(attemptException)); } assertTrue(!isFirstAttempt() || exception == null); - Throwable newlyChosenException = transformException(exception, attemptException, onlyRuntimeExceptions, exceptionTransformer); + Throwable newlyChosenException = callOnAttemptFailureOperator(exception, attemptException, onlyRuntimeExceptions, onAttemptFailureOperator); if (isLastAttempt()) { exception = newlyChosenException; throw exception; @@ -167,27 +169,31 @@ private void doAdvanceOrThrow(final Throwable attemptException, /** * @param onlyRuntimeExceptions See {@link #doAdvanceOrThrow(Throwable, BinaryOperator, BiPredicate, boolean)}. + * @param onAttemptFailureOperator See {@link #advanceOrThrow(RuntimeException, BinaryOperator, BiPredicate)}. */ - private static Throwable transformException(@Nullable final Throwable previouslyChosenException, final Throwable attemptException, - final boolean onlyRuntimeExceptions, final BinaryOperator exceptionTransformer) { + private static Throwable callOnAttemptFailureOperator( + @Nullable final Throwable previouslyChosenException, + final Throwable attemptException, + final boolean onlyRuntimeExceptions, + final BinaryOperator onAttemptFailureOperator) { if (onlyRuntimeExceptions && previouslyChosenException != null) { assertTrue(isRuntime(previouslyChosenException)); } Throwable result; try { - result = assertNotNull(exceptionTransformer.apply(previouslyChosenException, attemptException)); + result = assertNotNull(onAttemptFailureOperator.apply(previouslyChosenException, attemptException)); if (onlyRuntimeExceptions) { assertTrue(isRuntime(result)); } - } catch (Throwable exceptionTransformerException) { - if (onlyRuntimeExceptions && !isRuntime(exceptionTransformerException)) { - throw exceptionTransformerException; + } catch (Throwable onAttemptFailureOperatorException) { + if (onlyRuntimeExceptions && !isRuntime(onAttemptFailureOperatorException)) { + throw onAttemptFailureOperatorException; } if (previouslyChosenException != null) { - exceptionTransformerException.addSuppressed(previouslyChosenException); + onAttemptFailureOperatorException.addSuppressed(previouslyChosenException); } - exceptionTransformerException.addSuppressed(attemptException); - throw exceptionTransformerException; + onAttemptFailureOperatorException.addSuppressed(attemptException); + throw onAttemptFailureOperatorException; } return result; } diff --git a/driver-core/src/main/com/mongodb/internal/async/function/RetryingAsyncCallbackSupplier.java b/driver-core/src/main/com/mongodb/internal/async/function/RetryingAsyncCallbackSupplier.java index e0f3d8c7457..16f6f2e7086 100644 --- a/driver-core/src/main/com/mongodb/internal/async/function/RetryingAsyncCallbackSupplier.java +++ b/driver-core/src/main/com/mongodb/internal/async/function/RetryingAsyncCallbackSupplier.java @@ -41,31 +41,34 @@ public final class RetryingAsyncCallbackSupplier implements AsyncCallbackSupplier { private final RetryState state; private final BiPredicate retryPredicate; - private final BinaryOperator failedResultTransformer; + private final BinaryOperator onAttemptFailureOperator; private final AsyncCallbackSupplier asyncFunction; /** * @param state The {@link RetryState} to be deemed as initial for the purpose of the new {@link RetryingAsyncCallbackSupplier}. - * @param failedResultTransformer A function that chooses which failed result of the {@code asyncFunction} to preserve as a prospective - * failed result of this {@link RetryingAsyncCallbackSupplier} and may also transform or mutate the exceptions. - * The choice is between + * @param onAttemptFailureOperator The action that is called once per failed attempt before (in the happens-before order) the + * {@code retryPredicate}, regardless of whether the {@code retryPredicate} is called. + * This action is allowed to have side effects. + *

    + * It also has to choose which exception to preserve as a prospective failed result of this {@link RetryingAsyncCallbackSupplier}. + * The {@code onAttemptFailureOperator} may mutate its arguments, choose from the arguments, or return a different exception, + * but it must return a {@code @}{@link NonNull} value. + * The choice is between

    *
      *
    • the previously chosen failed result or {@code null} if none has been chosen - * (the first argument of the {@code failedResultTransformer})
      • - *
    • and the failed result from the most recent attempt (the second argument of the {@code failedResultTransformer}).
      • + * (the first argument of the {@code onAttemptFailureOperator}) + *
    • and the failed result from the most recent attempt (the second argument of the {@code onAttemptFailureOperator}).
      • *
    - * The {@code failedResultTransformer} may either choose from its arguments, or return a different exception, a.k.a. transform, - * but it must return a {@code @}{@link NonNull} value. - * If it completes abruptly, then the {@code asyncFunction} cannot be retried and the exception thrown by - * the {@code failedResultTransformer} is used as a failed result of this {@link RetryingAsyncCallbackSupplier}. - * The {@code failedResultTransformer} is called before (in the happens-before order) the {@code retryPredicate}. - * The result of the {@code failedResultTransformer} does not affect what exception is passed to the {@code retryPredicate}. + * The result of the {@code onAttemptFailureOperator} does not affect the exception passed to the {@code retryPredicate}. + *

    + * If {@code onAttemptFailureOperator} completes abruptly, then the {@code asyncFunction} cannot be retried and the exception thrown by + * the {@code onAttemptFailureOperator} is used as a failed result of this {@link RetryingAsyncCallbackSupplier}.

    * @param retryPredicate {@code true} iff another attempt needs to be made. If it completes abruptly, * then the {@code asyncFunction} cannot be retried and the exception thrown by the {@code retryPredicate} * is used as a failed result of this {@link RetryingAsyncCallbackSupplier}. The {@code retryPredicate} is called not more than once * per attempt and only if all the following is true: *
      - *
    • {@code failedResultTransformer} completed normally;
      • + *
    • {@code onAttemptFailureOperator} completed normally;
      • *
    • the most recent attempt is not the {@linkplain RetryState#isLastAttempt() last} one.
      • *
    * The {@code retryPredicate} accepts this {@link RetryState} and the exception from the most recent attempt, @@ -75,12 +78,12 @@ public final class RetryingAsyncCallbackSupplier implements AsyncCallbackSupp */ public RetryingAsyncCallbackSupplier( final RetryState state, - final BinaryOperator failedResultTransformer, + final BinaryOperator onAttemptFailureOperator, final BiPredicate retryPredicate, final AsyncCallbackSupplier asyncFunction) { this.state = state; this.retryPredicate = retryPredicate; - this.failedResultTransformer = failedResultTransformer; + this.onAttemptFailureOperator = onAttemptFailureOperator; this.asyncFunction = asyncFunction; } @@ -113,7 +116,7 @@ private class RetryingCallback implements SingleResultCallback { public void onResult(@Nullable final R result, @Nullable final Throwable t) { if (t != null) { try { - state.advanceOrThrow(t, failedResultTransformer, retryPredicate); + state.advanceOrThrow(t, onAttemptFailureOperator, retryPredicate); } catch (Throwable failedResult) { wrapped.onResult(null, failedResult); return; diff --git a/driver-core/src/main/com/mongodb/internal/async/function/RetryingSyncSupplier.java b/driver-core/src/main/com/mongodb/internal/async/function/RetryingSyncSupplier.java index 315197f0da9..ad3e4b2b807 100644 --- a/driver-core/src/main/com/mongodb/internal/async/function/RetryingSyncSupplier.java +++ b/driver-core/src/main/com/mongodb/internal/async/function/RetryingSyncSupplier.java @@ -37,26 +37,26 @@ public final class RetryingSyncSupplier implements Supplier { private final RetryState state; private final BiPredicate retryPredicate; - private final BinaryOperator failedResultTransformer; + private final BinaryOperator onAttemptFailureOperator; private final Supplier syncFunction; /** * See {@link RetryingAsyncCallbackSupplier#RetryingAsyncCallbackSupplier(RetryState, BinaryOperator, BiPredicate, AsyncCallbackSupplier)} * for the documentation of the parameters. * - * @param failedResultTransformer Even though the {@code failedResultTransformer} accepts {@link Throwable}, + * @param onAttemptFailureOperator Even though the {@code onAttemptFailureOperator} accepts {@link Throwable}, * only {@link RuntimeException}s are passed to it. * @param retryPredicate Even though the {@code retryPredicate} accepts {@link Throwable}, * only {@link RuntimeException}s are passed to it. */ public RetryingSyncSupplier( final RetryState state, - final BinaryOperator failedResultTransformer, + final BinaryOperator onAttemptFailureOperator, final BiPredicate retryPredicate, final Supplier syncFunction) { this.state = state; this.retryPredicate = retryPredicate; - this.failedResultTransformer = failedResultTransformer; + this.onAttemptFailureOperator = onAttemptFailureOperator; this.syncFunction = syncFunction; } @@ -66,10 +66,10 @@ public R get() { try { return syncFunction.get(); } catch (RuntimeException attemptException) { - state.advanceOrThrow(attemptException, failedResultTransformer, retryPredicate); + state.advanceOrThrow(attemptException, onAttemptFailureOperator, retryPredicate); } catch (Exception attemptException) { // wrap potential sneaky / Kotlin exceptions - state.advanceOrThrow(new RuntimeException(attemptException), failedResultTransformer, retryPredicate); + state.advanceOrThrow(new RuntimeException(attemptException), onAttemptFailureOperator, retryPredicate); } } } From d25010d928704dcb7f29921f10f70505b4d45d9f Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Mon, 6 May 2024 11:53:27 -0600 Subject: [PATCH 10/19] Refactor the way `BaseCluster.selectServer` deals with the race condition The new approach allows us to later refactor all other logic inside one or more `ServerSelector`s. See the comment left in the code for more details on the new approach. JAVA-4254 --- .../AbstractMultiServerCluster.java | 15 ++++---- .../internal/connection/BaseCluster.java | 38 ++++++++++++++----- .../mongodb/internal/connection/Cluster.java | 21 +++++++--- .../connection/LoadBalancedCluster.java | 5 ++- .../connection/SingleServerCluster.java | 6 +-- ...tractServerDiscoveryAndMonitoringTest.java | 5 ++- .../BaseClusterSpecification.groovy | 7 +++- .../DefaultServerSpecification.groovy | 7 +++- .../MultiServerClusterSpecification.groovy | 4 +- .../ServerDiscoveryAndMonitoringTest.java | 2 +- ...erverSelectionWithinLatencyWindowTest.java | 12 +++--- .../SingleServerClusterSpecification.groovy | 6 +-- 12 files changed, 82 insertions(+), 46 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/AbstractMultiServerCluster.java b/driver-core/src/main/com/mongodb/internal/connection/AbstractMultiServerCluster.java index 5b6fa8f56fe..6783b85948d 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/AbstractMultiServerCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/AbstractMultiServerCluster.java @@ -31,9 +31,11 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -122,14 +124,13 @@ public void close() { } @Override - public ClusterableServer getServer(final ServerAddress serverAddress) { + public ServersSnapshot getServersSnapshot() { isTrue("is open", !isClosed()); - - ServerTuple serverTuple = addressToServerTupleMap.get(serverAddress); - if (serverTuple == null) { - return null; - } - return serverTuple.server; + Map nonAtomicSnapshot = new HashMap<>(addressToServerTupleMap); + return serverAddress -> { + ServerTuple serverTuple = nonAtomicSnapshot.get(serverAddress); + return serverTuple == null ? null : serverTuple.server; + }; } void onChange(final Collection newHosts) { diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index 8bebadb8dba..fd91ebde958 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -58,6 +58,7 @@ import java.util.concurrent.locks.ReentrantLock; import java.util.function.Function; +import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.assertions.Assertions.isTrue; import static com.mongodb.assertions.Assertions.notNull; import static com.mongodb.connection.ServerDescription.MAX_DRIVER_WIRE_VERSION; @@ -314,16 +315,35 @@ private boolean handleServerSelectionRequest(final ServerSelectionRequest reques @Nullable private ServerTuple selectServer(final ServerSelector serverSelector, final ClusterDescription clusterDescription) { - return selectServer(serverSelector, clusterDescription, this::getServer); + return selectServer(serverSelector, clusterDescription, getServersSnapshot()); } @Nullable @VisibleForTesting(otherwise = PRIVATE) static ServerTuple selectServer(final ServerSelector serverSelector, final ClusterDescription clusterDescription, - final Function serverCatalog) { - return atMostNRandom(new ArrayList<>(serverSelector.select(clusterDescription)), 2, serverDescription -> { - Server server = serverCatalog.apply(serverDescription.getAddress()); - return server == null ? null : new ServerTuple(server, serverDescription); + final ServersSnapshot serversSnapshot) { + // The set of `Server`s maintained by the `Cluster` is updated concurrently with `clusterDescription` being read. + // Additionally, that set of servers continues to be concurrently updated while `serverSelector` selects. + // This race condition means that we are not guaranteed not observe all the servers from `clusterDescription` + // among the `Server`s maintained by the `Cluster`. + // To deal with this race condition, we take `serversSnapshot` of that set of `Server`s + // (the snapshot itself does not have to be atomic) non-atomically with reading `clusterDescription` + // (this means, `serversSnapshot` and `clusterDescription` are not guaranteed to be consistent with each other), + // and do pre-filtering to make sure that the only `ServerDescription`s we may select, + // are of those `Server`s that are known to both `clusterDescription` and `serversSnapshot`. + // This way we are guaranteed to successfully get `Server`s from `serversSnapshot` based on the selected `ServerDescription`s. + // + // The pre-filtering we do to deal with the race condition described above is achieved by this `ServerSelector`. + ServerSelector raceConditionPreFiltering = clusterDescriptionPotentiallyInconsistentWithServerSnapshot -> + clusterDescriptionPotentiallyInconsistentWithServerSnapshot.getServerDescriptions() + .stream() + .filter(serverDescription -> serversSnapshot.containsServer(serverDescription.getAddress())) + .collect(toList()); + List intermediateResult = new CompositeServerSelector(asList(raceConditionPreFiltering, serverSelector)) + .select(clusterDescription); + return atMostNRandom(new ArrayList<>(intermediateResult), 2, serverDescription -> { + Server server = assertNotNull(serversSnapshot.getServer(serverDescription.getAddress())); + return new ServerTuple(server, serverDescription); }).stream() .min(comparingInt(serverTuple -> serverTuple.getServer().operationCount())) .orElse(null); @@ -345,10 +365,8 @@ private static List atMostNRandom(final ArrayList result = new ArrayList<>(n); for (int i = list.size() - 1; i >= 0 && result.size() < n; i--) { Collections.swap(list, i, random.nextInt(i + 1)); - ServerTuple serverTuple = transformer.apply(list.get(i)); - if (serverTuple != null) { - result.add(serverTuple); - } + ServerTuple serverTuple = assertNotNull(transformer.apply(list.get(i))); + result.add(serverTuple); } return result; } @@ -356,7 +374,7 @@ private static List atMostNRandom(final ArrayList selectors = Stream.of( serverSelector, - settings.getServerSelector(), + settings.getServerSelector(), // may be null new LatencyMinimizingServerSelector(settings.getLocalThreshold(MILLISECONDS), MILLISECONDS) ).filter(Objects::nonNull).collect(toList()); return serverDeprioritization.apply(new CompositeServerSelector(selectors)); diff --git a/driver-core/src/main/com/mongodb/internal/connection/Cluster.java b/driver-core/src/main/com/mongodb/internal/connection/Cluster.java index a3a649b10a6..358eb90a175 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/Cluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/Cluster.java @@ -18,9 +18,9 @@ import com.mongodb.ServerAddress; +import com.mongodb.annotations.ThreadSafe; import com.mongodb.connection.ClusterId; import com.mongodb.event.ServerDescriptionChangedEvent; -import com.mongodb.internal.VisibleForTesting; import com.mongodb.internal.async.SingleResultCallback; import com.mongodb.connection.ClusterDescription; import com.mongodb.connection.ClusterSettings; @@ -29,8 +29,6 @@ import java.io.Closeable; -import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE; - /** * Represents a cluster of MongoDB servers. Implementations can define the behaviour depending upon the type of cluster. * @@ -43,9 +41,7 @@ public interface Cluster extends Closeable { ClusterId getClusterId(); - @Nullable - @VisibleForTesting(otherwise = PRIVATE) - ClusterableServer getServer(ServerAddress serverAddress); + ServersSnapshot getServersSnapshot(); /** * Get the current description of this cluster. @@ -89,4 +85,17 @@ void selectServerAsync(ServerSelector serverSelector, OperationContext operation * Server Discovery And Monitoring specification. */ void onChange(ServerDescriptionChangedEvent event); + + /** + * A non-atomic snapshot of the servers in a {@link Cluster}. + */ + @ThreadSafe + interface ServersSnapshot { + @Nullable + Server getServer(ServerAddress serverAddress); + + default boolean containsServer(final ServerAddress serverAddress) { + return getServer(serverAddress) != null; + } + } } diff --git a/driver-core/src/main/com/mongodb/internal/connection/LoadBalancedCluster.java b/driver-core/src/main/com/mongodb/internal/connection/LoadBalancedCluster.java index dff239ab204..efc6c4bfb47 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/LoadBalancedCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/LoadBalancedCluster.java @@ -181,10 +181,11 @@ public ClusterId getClusterId() { } @Override - public ClusterableServer getServer(final ServerAddress serverAddress) { + public ServersSnapshot getServersSnapshot() { isTrue("open", !isClosed()); waitForSrv(); - return assertNotNull(server); + ClusterableServer server = assertNotNull(this.server); + return serverAddress -> server; } @Override diff --git a/driver-core/src/main/com/mongodb/internal/connection/SingleServerCluster.java b/driver-core/src/main/com/mongodb/internal/connection/SingleServerCluster.java index ce76522ac1d..3c9d3b126bf 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/SingleServerCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/SingleServerCluster.java @@ -17,7 +17,6 @@ package com.mongodb.internal.connection; import com.mongodb.MongoConfigurationException; -import com.mongodb.ServerAddress; import com.mongodb.connection.ClusterConnectionMode; import com.mongodb.connection.ClusterDescription; import com.mongodb.connection.ClusterId; @@ -69,9 +68,10 @@ protected void connect() { } @Override - public ClusterableServer getServer(final ServerAddress serverAddress) { + public ServersSnapshot getServersSnapshot() { isTrue("open", !isClosed()); - return assertNotNull(server.get()); + ClusterableServer server = assertNotNull(this.server.get()); + return serverAddress -> server; } @Override diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/AbstractServerDiscoveryAndMonitoringTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/AbstractServerDiscoveryAndMonitoringTest.java index 5ac3c35d4ae..c0924c3d74d 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/AbstractServerDiscoveryAndMonitoringTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/AbstractServerDiscoveryAndMonitoringTest.java @@ -81,12 +81,13 @@ protected void applyResponse(final BsonArray response) { protected void applyApplicationError(final BsonDocument applicationError) { ServerAddress serverAddress = new ServerAddress(applicationError.getString("address").getValue()); int errorGeneration = applicationError.getNumber("generation", - new BsonInt32(((DefaultServer) getCluster().getServer(serverAddress)).getConnectionPool().getGeneration())).intValue(); + new BsonInt32(((DefaultServer) getCluster().getServersSnapshot().getServer(serverAddress)) + .getConnectionPool().getGeneration())).intValue(); int maxWireVersion = applicationError.getNumber("maxWireVersion").intValue(); String when = applicationError.getString("when").getValue(); String type = applicationError.getString("type").getValue(); - DefaultServer server = (DefaultServer) cluster.getServer(serverAddress); + DefaultServer server = (DefaultServer) cluster.getServersSnapshot().getServer(serverAddress); RuntimeException exception; switch (type) { diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy index c7428d2f4e7..9cbc8ce01d4 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy @@ -67,8 +67,11 @@ class BaseClusterSpecification extends Specification { } @Override - ClusterableServer getServer(final ServerAddress serverAddress) { - throw new UnsupportedOperationException() + Cluster.ServersSnapshot getServersSnapshot() { + Cluster.ServersSnapshot result = serverAddress -> { + throw new UnsupportedOperationException() + } + return result } @Override diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy index e849f8fa203..418fe5841ff 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy @@ -394,8 +394,11 @@ class DefaultServerSpecification extends Specification { } @Override - ClusterableServer getServer(final ServerAddress serverAddress) { - throw new UnsupportedOperationException() + Cluster.ServersSnapshot getServersSnapshot() { + Cluster.ServersSnapshot result = serverAddress -> { + throw new UnsupportedOperationException() + } + return result } @Override diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/MultiServerClusterSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/MultiServerClusterSpecification.groovy index 096053a0b11..f14305bb6b8 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/MultiServerClusterSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/MultiServerClusterSpecification.groovy @@ -87,14 +87,14 @@ class MultiServerClusterSpecification extends Specification { cluster.getCurrentDescription().connectionMode == MULTIPLE } - def 'should not get server when closed'() { + def 'should not get servers snapshot when closed'() { given: def cluster = new MultiServerCluster(CLUSTER_ID, ClusterSettings.builder().hosts(Arrays.asList(firstServer)).mode(MULTIPLE).build(), factory) cluster.close() when: - cluster.getServer(firstServer) + cluster.getServersSnapshot() then: thrown(IllegalStateException) diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDiscoveryAndMonitoringTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDiscoveryAndMonitoringTest.java index 4af47cb9557..2bc41fee1be 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDiscoveryAndMonitoringTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDiscoveryAndMonitoringTest.java @@ -120,7 +120,7 @@ private void assertServer(final String serverName, final BsonDocument expectedSe if (expectedServerDescriptionDocument.isDocument("pool")) { int expectedGeneration = expectedServerDescriptionDocument.getDocument("pool").getNumber("generation").intValue(); - DefaultServer server = (DefaultServer) getCluster().getServer(new ServerAddress(serverName)); + DefaultServer server = (DefaultServer) getCluster().getServersSnapshot().getServer(new ServerAddress(serverName)); assertEquals(expectedGeneration, server.getConnectionPool().getGeneration()); } } diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java index dd1c95c5e59..df5b9b63226 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java @@ -56,7 +56,7 @@ @RunWith(Parameterized.class) public class ServerSelectionWithinLatencyWindowTest { private final ClusterDescription clusterDescription; - private final Map serverCatalog; + private final Cluster.ServersSnapshot serversSnapshot; private final int iterations; private final Outcome outcome; @@ -65,7 +65,7 @@ public ServerSelectionWithinLatencyWindowTest( @SuppressWarnings("unused") final String description, final BsonDocument definition) { clusterDescription = buildClusterDescription(definition.getDocument("topology_description"), null); - serverCatalog = serverCatalog(definition.getArray("mocked_topology_state")); + serversSnapshot = serverCatalog(definition.getArray("mocked_topology_state")); iterations = definition.getInt32("iterations").getValue(); outcome = Outcome.parse(definition.getDocument("outcome")); } @@ -74,8 +74,7 @@ public ServerSelectionWithinLatencyWindowTest( public void shouldPassAllOutcomes() { ServerSelector selector = new ReadPreferenceServerSelector(ReadPreference.nearest()); Map> selectionResultsGroupedByServerAddress = IntStream.range(0, iterations) - .mapToObj(i -> BaseCluster.selectServer(selector, clusterDescription, - address -> Assertions.assertNotNull(serverCatalog.get(address)))) + .mapToObj(i -> BaseCluster.selectServer(selector, clusterDescription, serversSnapshot)) .collect(groupingBy(serverTuple -> serverTuple.getServerDescription().getAddress())); Map selectionFrequencies = selectionResultsGroupedByServerAddress.entrySet() .stream() @@ -97,8 +96,8 @@ public static Collection data() { .collect(toList()); } - private static Map serverCatalog(final BsonArray mockedTopologyState) { - return mockedTopologyState.stream() + private static Cluster.ServersSnapshot serverCatalog(final BsonArray mockedTopologyState) { + Map serverMap = mockedTopologyState.stream() .map(BsonValue::asDocument) .collect(toMap( el -> new ServerAddress(el.getString("address").getValue()), @@ -108,6 +107,7 @@ private static Map serverCatalog(final BsonArray mockedTo when(server.operationCount()).thenReturn(operationCount); return server; })); + return serverAddress -> Assertions.assertNotNull(serverMap.get(serverAddress)); } private static final class Outcome { diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/SingleServerClusterSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/SingleServerClusterSpecification.groovy index f47ab6644d8..a3a0f6a2d6f 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/SingleServerClusterSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/SingleServerClusterSpecification.groovy @@ -76,21 +76,21 @@ class SingleServerClusterSpecification extends Specification { sendNotification(firstServer, STANDALONE) then: - cluster.getServer(firstServer) == factory.getServer(firstServer) + cluster.getServersSnapshot().getServer(firstServer) == factory.getServer(firstServer) cleanup: cluster?.close() } - def 'should not get server when closed'() { + def 'should not get servers snapshot when closed'() { given: def cluster = new SingleServerCluster(CLUSTER_ID, ClusterSettings.builder().mode(SINGLE).hosts(Arrays.asList(firstServer)).build(), factory) cluster.close() when: - cluster.getServer(firstServer) + cluster.getServersSnapshot() then: thrown(IllegalStateException) From cc8021e3f1b97f041135cd53ec32543bf30d6f0f Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Tue, 7 May 2024 04:20:26 -0600 Subject: [PATCH 11/19] Refactor the server selection logic that is implemented not as a `ServerSelector` JAVA-4254 --- .../internal/connection/BaseCluster.java | 99 ++++++------- .../AtMostTwoRandomServerSelector.java | 57 ++++++++ ...perationCountMinimizingServerSelector.java | 62 ++++++++ .../BaseClusterSpecification.groovy | 6 +- .../DefaultServerSpecification.groovy | 6 +- ...erverSelectionWithinLatencyWindowTest.java | 6 +- .../AtMostTwoRandomServerSelectorTest.java | 99 +++++++++++++ ...tionCountMinimizingServerSelectorTest.java | 136 ++++++++++++++++++ 8 files changed, 412 insertions(+), 59 deletions(-) create mode 100644 driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java create mode 100644 driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java create mode 100644 driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java create mode 100644 driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index fd91ebde958..8508715a698 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -39,12 +39,13 @@ import com.mongodb.internal.logging.LogMessage; import com.mongodb.internal.logging.LogMessage.Entry; import com.mongodb.internal.logging.StructuredLogger; +import com.mongodb.internal.selector.AtMostTwoRandomServerSelector; import com.mongodb.internal.selector.LatencyMinimizingServerSelector; +import com.mongodb.internal.selector.OperationCountMinimizingServerSelector; import com.mongodb.lang.Nullable; import com.mongodb.selector.CompositeServerSelector; import com.mongodb.selector.ServerSelector; -import java.util.ArrayList; import java.util.Collections; import java.util.Deque; import java.util.Iterator; @@ -53,10 +54,8 @@ import java.util.stream.Stream; import java.util.concurrent.ConcurrentLinkedDeque; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Function; import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.assertions.Assertions.isTrue; @@ -82,7 +81,6 @@ import static com.mongodb.internal.thread.InterruptionUtil.interruptAndCreateMongoInterruptedException; import static java.lang.String.format; import static java.util.Arrays.asList; -import static java.util.Comparator.comparingInt; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.NANOSECONDS; import static java.util.stream.Collectors.toList; @@ -128,8 +126,7 @@ public ServerTuple selectServer(final ServerSelector serverSelector, final Opera ClusterDescription curDescription = description; logServerSelectionStarted(clusterId, operationContext, serverSelector, curDescription); ServerDeprioritization serverDeprioritization = operationContext.getServerDeprioritization(); - ServerSelector completeServerSelector = getCompleteServerSelector(serverSelector, serverDeprioritization); - ServerTuple serverTuple = selectServer(completeServerSelector, curDescription); + ServerTuple serverTuple = createCompleteSelectorAndSelectServer(serverSelector, curDescription, serverDeprioritization); boolean selectionWaitingLogged = false; @@ -171,7 +168,7 @@ public ServerTuple selectServer(final ServerSelector serverSelector, final Opera currentPhase = phase.get(); curDescription = description; - serverTuple = selectServer(completeServerSelector, curDescription); + serverTuple = createCompleteSelectorAndSelectServer(serverSelector, curDescription, serverDeprioritization); } } catch (InterruptedException e) { @@ -189,7 +186,6 @@ public void selectServerAsync(final ServerSelector serverSelector, final Operati logServerSelectionStarted(clusterId, operationContext, serverSelector, currentDescription); ServerSelectionRequest request = new ServerSelectionRequest(operationContext, serverSelector, - getCompleteServerSelector(serverSelector, operationContext.getServerDeprioritization()), getMaxWaitTimeNanos(), callback); if (!handleServerSelectionRequest(request, currentPhase, currentDescription)) { @@ -285,12 +281,13 @@ private boolean handleServerSelectionRequest(final ServerSelectionRequest reques return true; } - ServerTuple serverTuple = selectServer(request.completeSelector, description); + ServerDeprioritization serverDeprioritization = request.operationContext.getServerDeprioritization(); + ServerTuple serverTuple = createCompleteSelectorAndSelectServer(request.originalSelector, description, serverDeprioritization); if (serverTuple != null) { ServerAddress serverAddress = serverTuple.getServerDescription().getAddress(); logServerSelectionSucceeded(clusterId, request.operationContext, serverAddress, request.originalSelector, description); - request.operationContext.getServerDeprioritization().updateCandidate(serverAddress, description.getType()); + serverDeprioritization.updateCandidate(serverAddress, description.getType()); request.onResult(serverTuple, null); return true; } @@ -313,18 +310,48 @@ private boolean handleServerSelectionRequest(final ServerSelectionRequest reques } @Nullable - private ServerTuple selectServer(final ServerSelector serverSelector, - final ClusterDescription clusterDescription) { - return selectServer(serverSelector, clusterDescription, getServersSnapshot()); + private ServerTuple createCompleteSelectorAndSelectServer( + final ServerSelector serverSelector, + final ClusterDescription clusterDescription, + final ServerDeprioritization serverDeprioritization) { + return createCompleteSelectorAndSelectServer( + serverSelector, clusterDescription, getServersSnapshot(), serverDeprioritization, settings); } @Nullable @VisibleForTesting(otherwise = PRIVATE) - static ServerTuple selectServer(final ServerSelector serverSelector, final ClusterDescription clusterDescription, + static ServerTuple createCompleteSelectorAndSelectServer( + final ServerSelector serverSelector, + final ClusterDescription clusterDescription, + final ServersSnapshot serversSnapshot, + final ServerDeprioritization serverDeprioritization, + final ClusterSettings settings) { + ServerSelector completeServerSelector = getCompleteServerSelector(serverSelector, serverDeprioritization, serversSnapshot, settings); + return doSelectServer(completeServerSelector, clusterDescription, serversSnapshot); + } + + @Nullable + private static ServerTuple doSelectServer( + final ServerSelector serverSelector, + final ClusterDescription clusterDescription, final ServersSnapshot serversSnapshot) { + return serverSelector.select(clusterDescription) + .stream() + .map(serverDescription -> new ServerTuple( + assertNotNull(serversSnapshot.getServer(serverDescription.getAddress())), + serverDescription)) + .findAny() + .orElse(null); + } + + private static ServerSelector getCompleteServerSelector( + final ServerSelector serverSelector, + final ServerDeprioritization serverDeprioritization, + final ServersSnapshot serversSnapshot, + final ClusterSettings settings) { // The set of `Server`s maintained by the `Cluster` is updated concurrently with `clusterDescription` being read. // Additionally, that set of servers continues to be concurrently updated while `serverSelector` selects. - // This race condition means that we are not guaranteed not observe all the servers from `clusterDescription` + // This race condition means that we are not guaranteed to observe all the servers from `clusterDescription` // among the `Server`s maintained by the `Cluster`. // To deal with this race condition, we take `serversSnapshot` of that set of `Server`s // (the snapshot itself does not have to be atomic) non-atomically with reading `clusterDescription` @@ -339,43 +366,13 @@ static ServerTuple selectServer(final ServerSelector serverSelector, final Clust .stream() .filter(serverDescription -> serversSnapshot.containsServer(serverDescription.getAddress())) .collect(toList()); - List intermediateResult = new CompositeServerSelector(asList(raceConditionPreFiltering, serverSelector)) - .select(clusterDescription); - return atMostNRandom(new ArrayList<>(intermediateResult), 2, serverDescription -> { - Server server = assertNotNull(serversSnapshot.getServer(serverDescription.getAddress())); - return new ServerTuple(server, serverDescription); - }).stream() - .min(comparingInt(serverTuple -> serverTuple.getServer().operationCount())) - .orElse(null); - } - - /** - * Returns a new {@link List} of at most {@code n} elements, where each element is a result of - * {@linkplain Function#apply(Object) applying} the {@code transformer} to a randomly picked element from the specified {@code list}, - * such that no element is picked more than once. If the {@code transformer} produces {@code null}, then another element is picked - * until either {@code n} transformed non-{@code null} elements are collected, or the {@code list} does not have - * unpicked elements left. - *

    - * Note that this method may reorder the {@code list}, as it uses the - * Fisher–Yates, a.k.a. Durstenfeld, shuffle algorithm. - */ - private static List atMostNRandom(final ArrayList list, final int n, - final Function transformer) { - ThreadLocalRandom random = ThreadLocalRandom.current(); - List result = new ArrayList<>(n); - for (int i = list.size() - 1; i >= 0 && result.size() < n; i--) { - Collections.swap(list, i, random.nextInt(i + 1)); - ServerTuple serverTuple = assertNotNull(transformer.apply(list.get(i))); - result.add(serverTuple); - } - return result; - } - - private ServerSelector getCompleteServerSelector(final ServerSelector serverSelector, final ServerDeprioritization serverDeprioritization) { List selectors = Stream.of( + raceConditionPreFiltering, serverSelector, settings.getServerSelector(), // may be null - new LatencyMinimizingServerSelector(settings.getLocalThreshold(MILLISECONDS), MILLISECONDS) + new LatencyMinimizingServerSelector(settings.getLocalThreshold(MILLISECONDS), MILLISECONDS), + AtMostTwoRandomServerSelector.instance(), + new OperationCountMinimizingServerSelector(serversSnapshot) ).filter(Objects::nonNull).collect(toList()); return serverDeprioritization.apply(new CompositeServerSelector(selectors)); } @@ -426,7 +423,6 @@ private MongoException createAndLogTimeoutException( private static final class ServerSelectionRequest { private final OperationContext operationContext; private final ServerSelector originalSelector; - private final ServerSelector completeSelector; @Nullable private final Long maxWaitTimeNanos; private final SingleResultCallback callback; @@ -434,13 +430,12 @@ private static final class ServerSelectionRequest { private CountDownLatch phase; ServerSelectionRequest(final OperationContext operationContext, - final ServerSelector serverSelector, final ServerSelector completeSelector, + final ServerSelector serverSelector, @Nullable final Long maxWaitTimeNanos, final SingleResultCallback callback) { this.operationContext = operationContext; this.originalSelector = serverSelector; - this.completeSelector = completeSelector; this.maxWaitTimeNanos = maxWaitTimeNanos; this.callback = callback; } diff --git a/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java b/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java new file mode 100644 index 00000000000..afbb899db27 --- /dev/null +++ b/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java @@ -0,0 +1,57 @@ +/* + * 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.selector; + +import com.mongodb.annotations.Immutable; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ServerDescription; +import com.mongodb.selector.ServerSelector; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.ThreadLocalRandom; + +/** + * {@linkplain #select(ClusterDescription) Selects} at most two {@link ServerDescription}s at random. This selector uses the + * Fisher–Yates, a.k.a. Durstenfeld, shuffle algorithm. + * + *

    This class is not part of the public API and may be removed or changed at any time

    + */ +@Immutable +public final class AtMostTwoRandomServerSelector implements ServerSelector { + private static final int N = 2; + private static final AtMostTwoRandomServerSelector INSTANCE = new AtMostTwoRandomServerSelector(); + + private AtMostTwoRandomServerSelector() { + } + + public static AtMostTwoRandomServerSelector instance() { + return INSTANCE; + } + + @Override + public List select(final ClusterDescription clusterDescription) { + List serverDescriptions = new ArrayList<>(clusterDescription.getServerDescriptions()); + List result = new ArrayList<>(); + ThreadLocalRandom random = ThreadLocalRandom.current(); + for (int i = serverDescriptions.size() - 1; i >= 0 && result.size() < N; i--) { + Collections.swap(serverDescriptions, i, random.nextInt(i + 1)); + result.add(serverDescriptions.get(i)); + } + return result; + } +} diff --git a/driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java b/driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java new file mode 100644 index 00000000000..1c0e446a17d --- /dev/null +++ b/driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java @@ -0,0 +1,62 @@ +/* + * 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.selector; + +import com.mongodb.ServerAddress; +import com.mongodb.annotations.ThreadSafe; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ServerDescription; +import com.mongodb.internal.connection.Cluster.ServersSnapshot; +import com.mongodb.internal.connection.Server; +import com.mongodb.selector.ServerSelector; + +import java.util.List; + +import static com.mongodb.assertions.Assertions.assertNotNull; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static java.util.Comparator.comparingInt; + +/** + * {@linkplain #select(ClusterDescription) Selects} at most one {@link ServerDescription} + * corresponding to a {@link ServersSnapshot#getServer(ServerAddress) server} with the smallest {@link Server#operationCount()}. + * + *

    This class is not part of the public API and may be removed or changed at any time

    + */ +@ThreadSafe +public final class OperationCountMinimizingServerSelector implements ServerSelector { + private final ServersSnapshot serversSnapshot; + + /** + * @param serversSnapshot Must {@linkplain ServersSnapshot#containsServer(ServerAddress) contain} {@link Server}s corresponding to + * {@linkplain ClusterDescription#getServerDescriptions() all} {@link ServerDescription}s + * in the {@link ClusterDescription} passed to {@link #select(ClusterDescription)}. + */ + public OperationCountMinimizingServerSelector(final ServersSnapshot serversSnapshot) { + this.serversSnapshot = serversSnapshot; + } + + @Override + public List select(final ClusterDescription clusterDescription) { + ServerDescription selected = clusterDescription.getServerDescriptions() + .stream() + .min(comparingInt(serverDescription -> + assertNotNull(serversSnapshot.getServer(serverDescription.getAddress())) + .operationCount())) + .orElse(null); + return selected == null ? emptyList() : singletonList(selected); + } +} diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy index 9cbc8ce01d4..4517a7d307d 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy @@ -68,10 +68,10 @@ class BaseClusterSpecification extends Specification { @Override Cluster.ServersSnapshot getServersSnapshot() { - Cluster.ServersSnapshot result = serverAddress -> { - throw new UnsupportedOperationException() + Cluster.ServersSnapshot result = { + serverAddress -> throw new UnsupportedOperationException() } - return result + result } @Override diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy index 418fe5841ff..a0b96706f0e 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy @@ -395,10 +395,10 @@ class DefaultServerSpecification extends Specification { @Override Cluster.ServersSnapshot getServersSnapshot() { - Cluster.ServersSnapshot result = serverAddress -> { - throw new UnsupportedOperationException() + Cluster.ServersSnapshot result = { + serverAddress -> throw new UnsupportedOperationException() } - return result + result } @Override diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java index df5b9b63226..6f1a9d25bb1 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerSelectionWithinLatencyWindowTest.java @@ -20,6 +20,7 @@ import com.mongodb.ServerAddress; import com.mongodb.assertions.Assertions; import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ClusterSettings; import com.mongodb.internal.selector.ReadPreferenceServerSelector; import com.mongodb.selector.ServerSelector; import org.bson.BsonArray; @@ -73,8 +74,11 @@ public ServerSelectionWithinLatencyWindowTest( @Test public void shouldPassAllOutcomes() { ServerSelector selector = new ReadPreferenceServerSelector(ReadPreference.nearest()); + OperationContext.ServerDeprioritization emptyServerDeprioritization = new OperationContext().getServerDeprioritization(); + ClusterSettings defaultClusterSettings = ClusterSettings.builder().build(); Map> selectionResultsGroupedByServerAddress = IntStream.range(0, iterations) - .mapToObj(i -> BaseCluster.selectServer(selector, clusterDescription, serversSnapshot)) + .mapToObj(i -> BaseCluster.createCompleteSelectorAndSelectServer(selector, clusterDescription, serversSnapshot, + emptyServerDeprioritization, defaultClusterSettings)) .collect(groupingBy(serverTuple -> serverTuple.getServerDescription().getAddress())); Map selectionFrequencies = selectionResultsGroupedByServerAddress.entrySet() .stream() diff --git a/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java b/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java new file mode 100644 index 00000000000..5be2c5aefdc --- /dev/null +++ b/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java @@ -0,0 +1,99 @@ +/* + * 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.selector; + +import com.mongodb.ServerAddress; +import com.mongodb.connection.ClusterConnectionMode; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ClusterType; +import com.mongodb.connection.ServerConnectionState; +import com.mongodb.connection.ServerDescription; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.stream.Stream; + +import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static java.util.stream.Collectors.toList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +final class AtMostTwoRandomServerSelectorTest { + @ParameterizedTest + @MethodSource("args") + void select( + final List hosts, + final int numberOfSelectIterations, + final double expectedCount, + final double frequencyTolerance, + final int expectedSelectedSize) { + ClusterDescription clusterDescription = clusterDescription(hosts); + HashMap actualCounters = new HashMap<>(); + for (int i = 0; i < numberOfSelectIterations; i++) { + List selected = AtMostTwoRandomServerSelector.instance().select(clusterDescription); + assertEquals(expectedSelectedSize, selected.size()); + selected.forEach(serverDescription -> actualCounters.merge(serverDescription.getAddress(), 1, Integer::sum)); + } + actualCounters.forEach((serverAddress, counter) -> + assertEquals( + expectedCount / numberOfSelectIterations, + (double) counter / numberOfSelectIterations, + frequencyTolerance, + () -> String.format("serverAddress=%s, counter=%d, actualCounters=%s", serverAddress, counter, actualCounters))); + } + + private static Stream args() { + int smallNumberOfSelectIterations = 10; + int largeNumberOfSelectIterations = 2_000; + int maxSelectedSize = 2; + return Stream.of( + arguments(emptyList(), + smallNumberOfSelectIterations, 0, 0, 0), + arguments(singletonList("1"), + smallNumberOfSelectIterations, smallNumberOfSelectIterations, 0, 1), + arguments(asList("1", "2"), + smallNumberOfSelectIterations, smallNumberOfSelectIterations, 0, maxSelectedSize), + arguments(asList("1", "2", "3"), + largeNumberOfSelectIterations, (double) maxSelectedSize * largeNumberOfSelectIterations / 3, 0.05, maxSelectedSize), + arguments(asList("1", "2", "3", "4", "5", "6", "7"), + largeNumberOfSelectIterations, (double) maxSelectedSize * largeNumberOfSelectIterations / 7, 0.05, maxSelectedSize) + ); + } + + private static ClusterDescription clusterDescription(final List hosts) { + return new ClusterDescription(ClusterConnectionMode.MULTIPLE, ClusterType.REPLICA_SET, serverDescriptions(hosts)); + } + + private static List serverDescriptions(final Collection hosts) { + return hosts.stream() + .map(AtMostTwoRandomServerSelectorTest::serverDescription) + .collect(toList()); + } + + private static ServerDescription serverDescription(final String host) { + return ServerDescription.builder() + .state(ServerConnectionState.CONNECTED) + .ok(true) + .address(new ServerAddress(host)) + .build(); + } +} diff --git a/driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java b/driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java new file mode 100644 index 00000000000..e5a8539d795 --- /dev/null +++ b/driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java @@ -0,0 +1,136 @@ +/* + * 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.selector; + +import com.mongodb.ServerAddress; +import com.mongodb.connection.ClusterConnectionMode; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ClusterType; +import com.mongodb.connection.ServerConnectionState; +import com.mongodb.connection.ServerDescription; +import com.mongodb.internal.connection.Cluster; +import com.mongodb.internal.connection.Server; +import com.mongodb.internal.mockito.MongoMockito; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; +import java.util.stream.Stream; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; +import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toMap; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.when; + +final class OperationCountMinimizingServerSelectorTest { + @ParameterizedTest + @MethodSource("args") + void select(final Map hostToOperationCount, final List expectedHosts) { + ClusterDescriptionAndServersSnapshot pair = clusterDescriptionAndServersSnapshot(hostToOperationCount); + List actualHosts = new OperationCountMinimizingServerSelector(pair.getServersSnapshot()) + .select(pair.getClusterDescription()) + .stream() + .map(serverDescription -> serverDescription.getAddress().getHost()) + .collect(toList()); + assertEquals(expectedHosts, actualHosts, hostToOperationCount::toString); + } + + private static Stream args() { + return Stream.of( + arguments(emptyMap(), emptyList()), + arguments(singletonMap("a", 0), singletonList("a")), + arguments(linkedMap(m -> { + m.put("b", 0); + m.put("a", 5); + }), singletonList("b")), + arguments(linkedMap(m -> { + m.put("b", 2); + m.put("a", 3); + m.put("c", 2); + }), singletonList("b")), + arguments(linkedMap(m -> { + m.put("b", 5); + m.put("a", 5); + m.put("e", 0); + m.put("c", 5); + m.put("d", 8); + }), singletonList("e")) + ); + } + + private static ClusterDescriptionAndServersSnapshot clusterDescriptionAndServersSnapshot(final Map hostToOperationCount) { + ClusterDescription clusterDescription = new ClusterDescription( + ClusterConnectionMode.MULTIPLE, ClusterType.REPLICA_SET, serverDescriptions(hostToOperationCount.keySet())); + Map serverAddressToOperationCount = hostToOperationCount.entrySet() + .stream().collect(toMap(entry -> new ServerAddress(entry.getKey()), Map.Entry::getValue)); + Cluster.ServersSnapshot serversSnapshot = serverAddress -> { + int operationCount = serverAddressToOperationCount.get(serverAddress); + return MongoMockito.mock(Server.class, server -> + when(server.operationCount()).thenReturn(operationCount)); + }; + return new ClusterDescriptionAndServersSnapshot(clusterDescription, serversSnapshot); + } + + private static List serverDescriptions(final Collection hosts) { + return hosts.stream() + .map(OperationCountMinimizingServerSelectorTest::serverDescription) + .collect(toList()); + } + + private static ServerDescription serverDescription(final String host) { + return ServerDescription.builder() + .state(ServerConnectionState.CONNECTED) + .ok(true) + .address(new ServerAddress(host)) + .build(); + } + + private static LinkedHashMap linkedMap(final Consumer> filler) { + LinkedHashMap result = new LinkedHashMap<>(); + filler.accept(result); + return result; + } + + private static final class ClusterDescriptionAndServersSnapshot { + private final ClusterDescription clusterDescription; + private final Cluster.ServersSnapshot serversSnapshot; + + private ClusterDescriptionAndServersSnapshot( + final ClusterDescription clusterDescription, + final Cluster.ServersSnapshot serversSnapshot) { + this.clusterDescription = clusterDescription; + this.serversSnapshot = serversSnapshot; + } + + ClusterDescription getClusterDescription() { + return clusterDescription; + } + + Cluster.ServersSnapshot getServersSnapshot() { + return serversSnapshot; + } + } +} From b3430bd0d60838fdeff00ef682857279b56d0747 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Tue, 7 May 2024 11:37:54 -0600 Subject: [PATCH 12/19] Implement `DeprioritizingSelector` strictly according to the spec requirements JAVA-4254 --- .../internal/connection/BaseCluster.java | 3 +- .../internal/connection/OperationContext.java | 47 +++++++--------- .../ServerDeprioritizationTest.java | 55 +++++-------------- 3 files changed, 35 insertions(+), 70 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index 8508715a698..4400d017642 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -369,12 +369,13 @@ private static ServerSelector getCompleteServerSelector( List selectors = Stream.of( raceConditionPreFiltering, serverSelector, + serverDeprioritization.getServerSelector(), settings.getServerSelector(), // may be null new LatencyMinimizingServerSelector(settings.getLocalThreshold(MILLISECONDS), MILLISECONDS), AtMostTwoRandomServerSelector.instance(), new OperationCountMinimizingServerSelector(serversSnapshot) ).filter(Objects::nonNull).collect(toList()); - return serverDeprioritization.apply(new CompositeServerSelector(selectors)); + return new CompositeServerSelector(selectors); } protected ClusterableServer createServer(final ServerAddress serverAddress) { diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index 03fb6ae71e2..b3ce3ca5e22 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; +import static java.util.stream.Collectors.toList; /** *

    This class is not part of the public API and may be removed or changed at any time

    @@ -56,14 +57,21 @@ public static final class ServerDeprioritization { @Nullable private ServerAddress candidate; private final Set deprioritized; + private final DeprioritizingSelector selector; private ServerDeprioritization() { candidate = null; deprioritized = new HashSet<>(); + selector = new DeprioritizingSelector(); } - ServerSelector apply(final ServerSelector selector) { - return new DeprioritizingSelector(selector); + /** + * The returned {@link ServerSelector} tries to {@linkplain ServerSelector#select(ClusterDescription) select} + * only the {@link ServerDescription}s that do not have deprioritized {@link ServerAddress}es. + * If no such {@link ServerDescription} can be selected, then it selects {@link ClusterDescription#getServerDescriptions()}. + */ + ServerSelector getServerSelector() { + return selector; } void updateCandidate(final ServerAddress serverAddress, final ClusterType clusterType) { @@ -88,39 +96,22 @@ private static boolean isEnabled(final ClusterType clusterType) { * which indeed may be used concurrently. {@link DeprioritizingSelector} does not need to be thread-safe. */ private final class DeprioritizingSelector implements ServerSelector { - private final ServerSelector wrapped; - - private DeprioritizingSelector(final ServerSelector wrapped) { - this.wrapped = wrapped; + private DeprioritizingSelector() { } @Override public List select(final ClusterDescription clusterDescription) { - if (isEnabled(clusterDescription.getType())) { - List nonDeprioritizedServerDescriptions = ClusterDescriptionHelper.getServersByPredicate( - clusterDescription, serverDescription -> !deprioritized.contains(serverDescription.getAddress())); - List result = wrapped.select( - copyWithServerDescriptions(clusterDescription, nonDeprioritizedServerDescriptions)); - if (result.isEmpty()) { - // fall back to selecting from all servers ignoring the deprioritized ones - result = wrapped.select(clusterDescription); - } - return result; + List serverDescriptions = clusterDescription.getServerDescriptions(); + if (!isEnabled(clusterDescription.getType())) { + return serverDescriptions; } else { - return wrapped.select(clusterDescription); + List nonDeprioritizedServerDescriptions = serverDescriptions + .stream() + .filter(serverDescription -> !deprioritized.contains(serverDescription.getAddress())) + .collect(toList()); + return nonDeprioritizedServerDescriptions.isEmpty() ? serverDescriptions : nonDeprioritizedServerDescriptions; } } - - private ClusterDescription copyWithServerDescriptions( - final ClusterDescription clusterDescription, final List serverDescriptions) { - return new ClusterDescription( - clusterDescription.getConnectionMode(), - clusterDescription.getType(), - clusterDescription.getSrvResolutionException(), - serverDescriptions, - clusterDescription.getClusterSettings(), - clusterDescription.getServerSettings()); - } } } } diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java index 88702c1e1be..2c2869e674d 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java @@ -25,8 +25,6 @@ import com.mongodb.connection.ServerDescription; import com.mongodb.connection.ServerId; import com.mongodb.internal.connection.OperationContext.ServerDeprioritization; -import com.mongodb.internal.selector.ServerAddressSelector; -import com.mongodb.selector.ServerSelector; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -35,7 +33,6 @@ import java.util.List; import static java.util.Arrays.asList; -import static java.util.Collections.singletonList; import static java.util.Collections.unmodifiableList; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; @@ -49,7 +46,6 @@ final class ServerDeprioritizationTest { private static final List ALL_SERVERS = unmodifiableList(asList(SERVER_A, SERVER_B, SERVER_C)); private static final ClusterDescription REPLICA_SET = clusterDescription(ClusterType.REPLICA_SET); private static final ClusterDescription SHARDED_CLUSTER = clusterDescription(ClusterType.SHARDED); - private static final ServerSelector ALL_SELECTOR = new AllServerSelector(); private ServerDeprioritization serverDeprioritization; @@ -59,43 +55,30 @@ void beforeEach() { } @Test - void applyNoneDeprioritized() { + void selectNoneDeprioritized() { assertAll( - () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)), - () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(REPLICA_SET)) + () -> assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)), + () -> assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(REPLICA_SET)) ); } @Test - void applySomeDeprioritized() { + void selectSomeDeprioritized() { deprioritize(SERVER_B); assertAll( - () -> assertEquals(asList(SERVER_A, SERVER_C), serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)), - () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(REPLICA_SET)) + () -> assertEquals(asList(SERVER_A, SERVER_C), serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)), + () -> assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(REPLICA_SET)) ); } @Test - void applyFallsBack() { + void selectAllDeprioritized() { + deprioritize(SERVER_A); + deprioritize(SERVER_B); + deprioritize(SERVER_C); assertAll( - () -> { - deprioritize(SERVER_A); - deprioritize(SERVER_B); - deprioritize(SERVER_C); - assertAll( - () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)), - () -> assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(REPLICA_SET)) - ); - }, - () -> { - ServerSelector serverBSelector = new ServerAddressSelector(SERVER_B.getAddress()); - deprioritize(SERVER_B); - assertAll( - () -> assertEquals(singletonList(SERVER_B), - serverDeprioritization.apply(serverBSelector).select(SHARDED_CLUSTER)), - () -> assertEquals(singletonList(SERVER_B), serverDeprioritization.apply(serverBSelector).select(REPLICA_SET)) - ); - } + () -> assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)), + () -> assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(REPLICA_SET)) ); } @@ -104,7 +87,7 @@ void applyFallsBack() { void updateCandidateIgnoresIfNotShardedCluster(final ClusterType clusterType) { serverDeprioritization.updateCandidate(SERVER_A.getAddress(), clusterType); serverDeprioritization.onAttemptFailure(new RuntimeException()); - assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)); + assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)); } @Test @@ -112,7 +95,7 @@ void onAttemptFailureIgnoresIfPoolClearedException() { serverDeprioritization.updateCandidate(SERVER_A.getAddress(), ClusterType.SHARDED); serverDeprioritization.onAttemptFailure( new MongoConnectionPoolClearedException(new ServerId(new ClusterId(), new ServerAddress()), null)); - assertEquals(ALL_SERVERS, serverDeprioritization.apply(ALL_SELECTOR).select(SHARDED_CLUSTER)); + assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)); } @Test @@ -138,14 +121,4 @@ private static ServerDescription serverDescription(final String host) { private static ClusterDescription clusterDescription(final ClusterType clusterType) { return new ClusterDescription(ClusterConnectionMode.MULTIPLE, clusterType, asList(SERVER_A, SERVER_B, SERVER_C)); } - - private static final class AllServerSelector implements ServerSelector { - AllServerSelector() { - } - - @Override - public List select(final ClusterDescription clusterDescription) { - return clusterDescription.getServerDescriptions(); - } - } } From 04880c7a70667a618a0724e482278baab86fac99 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Tue, 7 May 2024 14:07:11 -0600 Subject: [PATCH 13/19] Do minor touches JAVA-4254 --- .../mongodb/internal/connection/BaseCluster.java | 13 ++----------- .../selector/AtMostTwoRandomServerSelector.java | 4 ++-- .../selector/AtMostTwoRandomServerSelectorTest.java | 2 +- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index 4400d017642..22a179b85c4 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -185,8 +185,7 @@ public void selectServerAsync(final ServerSelector serverSelector, final Operati ClusterDescription currentDescription = description; logServerSelectionStarted(clusterId, operationContext, serverSelector, currentDescription); - ServerSelectionRequest request = new ServerSelectionRequest(operationContext, serverSelector, - getMaxWaitTimeNanos(), callback); + ServerSelectionRequest request = new ServerSelectionRequest(operationContext, serverSelector, getMaxWaitTimeNanos(), callback); if (!handleServerSelectionRequest(request, currentPhase, currentDescription)) { notifyWaitQueueHandler(request); @@ -327,15 +326,7 @@ static ServerTuple createCompleteSelectorAndSelectServer( final ServerDeprioritization serverDeprioritization, final ClusterSettings settings) { ServerSelector completeServerSelector = getCompleteServerSelector(serverSelector, serverDeprioritization, serversSnapshot, settings); - return doSelectServer(completeServerSelector, clusterDescription, serversSnapshot); - } - - @Nullable - private static ServerTuple doSelectServer( - final ServerSelector serverSelector, - final ClusterDescription clusterDescription, - final ServersSnapshot serversSnapshot) { - return serverSelector.select(clusterDescription) + return completeServerSelector.select(clusterDescription) .stream() .map(serverDescription -> new ServerTuple( assertNotNull(serversSnapshot.getServer(serverDescription.getAddress())), diff --git a/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java b/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java index afbb899db27..59fda64d961 100644 --- a/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java +++ b/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java @@ -33,7 +33,7 @@ */ @Immutable public final class AtMostTwoRandomServerSelector implements ServerSelector { - private static final int N = 2; + private static final int TWO = 2; private static final AtMostTwoRandomServerSelector INSTANCE = new AtMostTwoRandomServerSelector(); private AtMostTwoRandomServerSelector() { @@ -48,7 +48,7 @@ public List select(final ClusterDescription clusterDescriptio List serverDescriptions = new ArrayList<>(clusterDescription.getServerDescriptions()); List result = new ArrayList<>(); ThreadLocalRandom random = ThreadLocalRandom.current(); - for (int i = serverDescriptions.size() - 1; i >= 0 && result.size() < N; i--) { + for (int i = serverDescriptions.size() - 1; i >= 0 && result.size() < TWO; i--) { Collections.swap(serverDescriptions, i, random.nextInt(i + 1)); result.add(serverDescriptions.get(i)); } diff --git a/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java b/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java index 5be2c5aefdc..1d174ccabe7 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/selector/AtMostTwoRandomServerSelectorTest.java @@ -50,7 +50,7 @@ void select( HashMap actualCounters = new HashMap<>(); for (int i = 0; i < numberOfSelectIterations; i++) { List selected = AtMostTwoRandomServerSelector.instance().select(clusterDescription); - assertEquals(expectedSelectedSize, selected.size()); + assertEquals(expectedSelectedSize, selected.size(), selected::toString); selected.forEach(serverDescription -> actualCounters.merge(serverDescription.getAddress(), 1, Integer::sum)); } actualCounters.forEach((serverAddress, counter) -> From c1e9e4e3387810e8124f77061e9dd2b3ddcc14f3 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Tue, 7 May 2024 16:56:24 -0600 Subject: [PATCH 14/19] Update the documentation of `ClusterSettings.getServerSelector` JAVA-4254 --- .../com/mongodb/connection/ClusterSettings.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/driver-core/src/main/com/mongodb/connection/ClusterSettings.java b/driver-core/src/main/com/mongodb/connection/ClusterSettings.java index 84a24bbd22b..0af168725cd 100644 --- a/driver-core/src/main/com/mongodb/connection/ClusterSettings.java +++ b/driver-core/src/main/com/mongodb/connection/ClusterSettings.java @@ -468,16 +468,18 @@ public String getRequiredReplicaSetName() { * *

    The server selector augments the normal server selection rules applied by the driver when determining * which server to send an operation to. At the point that it's called by the driver, the - * {@link com.mongodb.connection.ClusterDescription} which is passed to it contains a list of - * {@link com.mongodb.connection.ServerDescription} instances which satisfy either the configured {@link com.mongodb.ReadPreference} - * for any read operation or ones that can take writes (e.g. a standalone, mongos, or replica set primary). + * {@link ClusterDescription} which is passed to it {@linkplain ClusterDescription#getServerDescriptions() contains} a list of + * {@link ServerDescription} instances which satisfy either the configured {@link com.mongodb.ReadPreference} + * for any read operation or ones that can take writes (e.g. a standalone, mongos, or replica set primary), + * barring those corresponding to servers that the driver considers unavailable or potentially problematic. *

    *

    The server selector can then filter the {@code ServerDescription} list using whatever criteria that is required by the * application.

    - *

    After this selector executes, two additional selectors are applied by the driver:

    + *

    After this selector executes, three additional selectors are applied by the driver:

    *
      *
    • select from within the latency window
      • - *
    • select a random server from those remaining
      • + *
    • select at most two random servers from those remaining
      • + *
    • select the one with fewer outstanding concurrent operations
      • *
    *

    To skip the latency window selector, an application can:

    *
      @@ -486,6 +488,7 @@ public String getRequiredReplicaSetName() { *
    * * @return the server selector, which may be null + * @see Builder#serverSelector(ServerSelector) */ @Nullable public ServerSelector getServerSelector() { From 246353f36dae51e28aa212488444ec54cb8ad5af Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Wed, 8 May 2024 17:04:30 -0600 Subject: [PATCH 15/19] Address review concerns JAVA-4254 --- .../internal/connection/BaseCluster.java | 8 +-- .../internal/connection/OperationContext.java | 23 ++++---- .../AtMostTwoRandomServerSelector.java | 5 +- ... MinimumOperationCountServerSelector.java} | 4 +- .../BaseClusterSpecification.groovy | 3 + .../internal/connection/BaseClusterTest.java | 58 +++++++++++++++++++ .../ServerDeprioritizationTest.java | 12 ++-- ...imumOperationCountServerSelectorTest.java} | 6 +- 8 files changed, 91 insertions(+), 28 deletions(-) rename driver-core/src/main/com/mongodb/internal/selector/{OperationCountMinimizingServerSelector.java => MinimumOperationCountServerSelector.java} (93%) create mode 100644 driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java rename driver-core/src/test/unit/com/mongodb/internal/selector/{OperationCountMinimizingServerSelectorTest.java => MinimumOperationCountServerSelectorTest.java} (95%) diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index 22a179b85c4..d7443d39c1b 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -41,7 +41,7 @@ import com.mongodb.internal.logging.StructuredLogger; import com.mongodb.internal.selector.AtMostTwoRandomServerSelector; import com.mongodb.internal.selector.LatencyMinimizingServerSelector; -import com.mongodb.internal.selector.OperationCountMinimizingServerSelector; +import com.mongodb.internal.selector.MinimumOperationCountServerSelector; import com.mongodb.lang.Nullable; import com.mongodb.selector.CompositeServerSelector; import com.mongodb.selector.ServerSelector; @@ -143,7 +143,7 @@ public ServerTuple selectServer(final ServerSelector serverSelector, final Opera ServerAddress serverAddress = serverTuple.getServerDescription().getAddress(); logServerSelectionSucceeded( clusterId, operationContext, serverAddress, serverSelector, curDescription); - serverDeprioritization.updateCandidate(serverAddress, curDescription.getType()); + serverDeprioritization.updateCandidate(serverAddress); return serverTuple; } @@ -286,7 +286,7 @@ private boolean handleServerSelectionRequest(final ServerSelectionRequest reques ServerAddress serverAddress = serverTuple.getServerDescription().getAddress(); logServerSelectionSucceeded(clusterId, request.operationContext, serverAddress, request.originalSelector, description); - serverDeprioritization.updateCandidate(serverAddress, description.getType()); + serverDeprioritization.updateCandidate(serverAddress); request.onResult(serverTuple, null); return true; } @@ -364,7 +364,7 @@ private static ServerSelector getCompleteServerSelector( settings.getServerSelector(), // may be null new LatencyMinimizingServerSelector(settings.getLocalThreshold(MILLISECONDS), MILLISECONDS), AtMostTwoRandomServerSelector.instance(), - new OperationCountMinimizingServerSelector(serversSnapshot) + new MinimumOperationCountServerSelector(serversSnapshot) ).filter(Objects::nonNull).collect(toList()); return new CompositeServerSelector(selectors); } diff --git a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java index b3ce3ca5e22..683f6adfbf8 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java +++ b/driver-core/src/main/com/mongodb/internal/connection/OperationContext.java @@ -74,8 +74,8 @@ ServerSelector getServerSelector() { return selector; } - void updateCandidate(final ServerAddress serverAddress, final ClusterType clusterType) { - candidate = isEnabled(clusterType) ? serverAddress : null; + void updateCandidate(final ServerAddress serverAddress) { + candidate = serverAddress; } public void onAttemptFailure(final Throwable failure) { @@ -86,10 +86,6 @@ public void onAttemptFailure(final Throwable failure) { deprioritized.add(candidate); } - private static boolean isEnabled(final ClusterType clusterType) { - return clusterType == ClusterType.SHARDED; - } - /** * {@link ServerSelector} requires thread safety, but that is only because a user may specify * {@link com.mongodb.connection.ClusterSettings.Builder#serverSelector(ServerSelector)}, @@ -104,13 +100,16 @@ public List select(final ClusterDescription clusterDescriptio List serverDescriptions = clusterDescription.getServerDescriptions(); if (!isEnabled(clusterDescription.getType())) { return serverDescriptions; - } else { - List nonDeprioritizedServerDescriptions = serverDescriptions - .stream() - .filter(serverDescription -> !deprioritized.contains(serverDescription.getAddress())) - .collect(toList()); - return nonDeprioritizedServerDescriptions.isEmpty() ? serverDescriptions : nonDeprioritizedServerDescriptions; } + List nonDeprioritizedServerDescriptions = serverDescriptions + .stream() + .filter(serverDescription -> !deprioritized.contains(serverDescription.getAddress())) + .collect(toList()); + return nonDeprioritizedServerDescriptions.isEmpty() ? serverDescriptions : nonDeprioritizedServerDescriptions; + } + + private boolean isEnabled(final ClusterType clusterType) { + return clusterType == ClusterType.SHARDED; } } } diff --git a/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java b/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java index 59fda64d961..22f55ac0245 100644 --- a/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java +++ b/driver-core/src/main/com/mongodb/internal/selector/AtMostTwoRandomServerSelector.java @@ -48,9 +48,12 @@ public List select(final ClusterDescription clusterDescriptio List serverDescriptions = new ArrayList<>(clusterDescription.getServerDescriptions()); List result = new ArrayList<>(); ThreadLocalRandom random = ThreadLocalRandom.current(); - for (int i = serverDescriptions.size() - 1; i >= 0 && result.size() < TWO; i--) { + for (int i = serverDescriptions.size() - 1; i >= 0; i--) { Collections.swap(serverDescriptions, i, random.nextInt(i + 1)); result.add(serverDescriptions.get(i)); + if (result.size() == TWO) { + break; + } } return result; } diff --git a/driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java b/driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java similarity index 93% rename from driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java rename to driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java index 1c0e446a17d..348b569aa9b 100644 --- a/driver-core/src/main/com/mongodb/internal/selector/OperationCountMinimizingServerSelector.java +++ b/driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java @@ -37,7 +37,7 @@ *

    This class is not part of the public API and may be removed or changed at any time

    */ @ThreadSafe -public final class OperationCountMinimizingServerSelector implements ServerSelector { +public final class MinimumOperationCountServerSelector implements ServerSelector { private final ServersSnapshot serversSnapshot; /** @@ -45,7 +45,7 @@ public final class OperationCountMinimizingServerSelector implements ServerSelec * {@linkplain ClusterDescription#getServerDescriptions() all} {@link ServerDescription}s * in the {@link ClusterDescription} passed to {@link #select(ClusterDescription)}. */ - public OperationCountMinimizingServerSelector(final ServersSnapshot serversSnapshot) { + public MinimumOperationCountServerSelector(final ServersSnapshot serversSnapshot) { this.serversSnapshot = serversSnapshot; } diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy index 4517a7d307d..0f51bab44a8 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy @@ -46,6 +46,9 @@ import static com.mongodb.connection.ServerType.REPLICA_SET_SECONDARY import static java.util.concurrent.TimeUnit.MILLISECONDS import static java.util.concurrent.TimeUnit.SECONDS +/** + * Add new tests to {@link BaseClusterTest}. + */ class BaseClusterSpecification extends Specification { private final ServerAddress firstServer = new ServerAddress('localhost:27017') diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java new file mode 100644 index 00000000000..f9b3dd6ff2d --- /dev/null +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java @@ -0,0 +1,58 @@ +/* + * 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.connection; + +import com.mongodb.ServerAddress; +import com.mongodb.connection.ClusterConnectionMode; +import com.mongodb.connection.ClusterDescription; +import com.mongodb.connection.ClusterSettings; +import com.mongodb.connection.ClusterType; +import com.mongodb.connection.ServerConnectionState; +import com.mongodb.connection.ServerDescription; +import com.mongodb.internal.mockito.MongoMockito; +import com.mongodb.internal.selector.ServerAddressSelector; +import org.junit.jupiter.api.Test; + +import static java.util.Arrays.asList; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.mockito.Mockito.when; + +final class BaseClusterTest { + @Test + void selectServerToleratesWhenThereIsNoServerForTheSelectedAddress() { + ServerAddress serverAddressA = new ServerAddress("a"); + ServerAddress serverAddressB = new ServerAddress("b"); + Server serverB = MongoMockito.mock(Server.class, server -> + when(server.operationCount()).thenReturn(0)); + ClusterDescription clusterDescriptionAB = new ClusterDescription(ClusterConnectionMode.MULTIPLE, ClusterType.SHARDED, + asList(serverDescription(serverAddressA), serverDescription(serverAddressB))); + Cluster.ServersSnapshot serversSnapshotB = serverAddress -> serverAddress.equals(serverAddressB) ? serverB : null; + assertDoesNotThrow(() -> BaseCluster.createCompleteSelectorAndSelectServer( + new ServerAddressSelector(serverAddressA), + clusterDescriptionAB, + serversSnapshotB, + new OperationContext().getServerDeprioritization(), + ClusterSettings.builder().build())); + } + + private static ServerDescription serverDescription(final ServerAddress serverAddress) { + return ServerDescription.builder() + .state(ServerConnectionState.CONNECTED) + .ok(true) + .address(serverAddress) + .build(); + } +} diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java index 2c2869e674d..816bca3f3f9 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/ServerDeprioritizationTest.java @@ -84,15 +84,15 @@ void selectAllDeprioritized() { @ParameterizedTest @EnumSource(value = ClusterType.class, mode = EXCLUDE, names = {"SHARDED"}) - void updateCandidateIgnoresIfNotShardedCluster(final ClusterType clusterType) { - serverDeprioritization.updateCandidate(SERVER_A.getAddress(), clusterType); + void serverSelectorSelectsAllIfNotShardedCluster(final ClusterType clusterType) { + serverDeprioritization.updateCandidate(SERVER_A.getAddress()); serverDeprioritization.onAttemptFailure(new RuntimeException()); - assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)); + assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(clusterDescription(clusterType))); } @Test void onAttemptFailureIgnoresIfPoolClearedException() { - serverDeprioritization.updateCandidate(SERVER_A.getAddress(), ClusterType.SHARDED); + serverDeprioritization.updateCandidate(SERVER_A.getAddress()); serverDeprioritization.onAttemptFailure( new MongoConnectionPoolClearedException(new ServerId(new ClusterId(), new ServerAddress()), null)); assertEquals(ALL_SERVERS, serverDeprioritization.getServerSelector().select(SHARDED_CLUSTER)); @@ -105,7 +105,7 @@ void onAttemptFailureDoesNotThrowIfNoCandidate() { private void deprioritize(final ServerDescription... serverDescriptions) { for (ServerDescription serverDescription : serverDescriptions) { - serverDeprioritization.updateCandidate(serverDescription.getAddress(), ClusterType.SHARDED); + serverDeprioritization.updateCandidate(serverDescription.getAddress()); serverDeprioritization.onAttemptFailure(new RuntimeException()); } } @@ -119,6 +119,6 @@ private static ServerDescription serverDescription(final String host) { } private static ClusterDescription clusterDescription(final ClusterType clusterType) { - return new ClusterDescription(ClusterConnectionMode.MULTIPLE, clusterType, asList(SERVER_A, SERVER_B, SERVER_C)); + return new ClusterDescription(ClusterConnectionMode.MULTIPLE, clusterType, ALL_SERVERS); } } diff --git a/driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java b/driver-core/src/test/unit/com/mongodb/internal/selector/MinimumOperationCountServerSelectorTest.java similarity index 95% rename from driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java rename to driver-core/src/test/unit/com/mongodb/internal/selector/MinimumOperationCountServerSelectorTest.java index e5a8539d795..3a0d754cb97 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/selector/OperationCountMinimizingServerSelectorTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/selector/MinimumOperationCountServerSelectorTest.java @@ -45,12 +45,12 @@ import static org.junit.jupiter.params.provider.Arguments.arguments; import static org.mockito.Mockito.when; -final class OperationCountMinimizingServerSelectorTest { +final class MinimumOperationCountServerSelectorTest { @ParameterizedTest @MethodSource("args") void select(final Map hostToOperationCount, final List expectedHosts) { ClusterDescriptionAndServersSnapshot pair = clusterDescriptionAndServersSnapshot(hostToOperationCount); - List actualHosts = new OperationCountMinimizingServerSelector(pair.getServersSnapshot()) + List actualHosts = new MinimumOperationCountServerSelector(pair.getServersSnapshot()) .select(pair.getClusterDescription()) .stream() .map(serverDescription -> serverDescription.getAddress().getHost()) @@ -96,7 +96,7 @@ private static ClusterDescriptionAndServersSnapshot clusterDescriptionAndServers private static List serverDescriptions(final Collection hosts) { return hosts.stream() - .map(OperationCountMinimizingServerSelectorTest::serverDescription) + .map(MinimumOperationCountServerSelectorTest::serverDescription) .collect(toList()); } From 69b78ffad40e07040c5f6c8c7e1b9d9e549bdc25 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Wed, 8 May 2024 17:42:20 -0600 Subject: [PATCH 16/19] Implement the proposed code simplification JAVA-4254 --- .../operation/AsyncOperationHelper.java | 15 ++++-------- .../operation/CommandOperationHelper.java | 23 +++++++++---------- .../operation/MixedBulkWriteOperation.java | 14 +++-------- .../operation/SyncOperationHelper.java | 15 ++++-------- 4 files changed, 22 insertions(+), 45 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java index 1a3d62ec792..b56f624bef5 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java @@ -44,7 +44,6 @@ import java.util.Collections; import java.util.List; -import java.util.function.BinaryOperator; import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.internal.async.ErrorHandlingResultCallback.errorHandlingCallback; @@ -53,6 +52,8 @@ import static com.mongodb.internal.operation.CommandOperationHelper.initialRetryState; import static com.mongodb.internal.operation.CommandOperationHelper.isRetryWritesEnabled; import static com.mongodb.internal.operation.CommandOperationHelper.logRetryExecute; +import static com.mongodb.internal.operation.CommandOperationHelper.onRetryableReadAttemptFailure; +import static com.mongodb.internal.operation.CommandOperationHelper.onRetryableWriteAttemptFailure; import static com.mongodb.internal.operation.CommandOperationHelper.transformWriteException; import static com.mongodb.internal.operation.WriteConcernHelper.throwOnWriteConcernError; @@ -286,11 +287,7 @@ static void createReadCommandAndExecuteAsync( static AsyncCallbackSupplier decorateReadWithRetriesAsync(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier asyncReadFunction) { - BinaryOperator onAttemptFailure = - (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> - CommandOperationHelper.onRetryableReadAttemptFailure( - operationContext, previouslyChosenException, mostRecentAttemptException); - return new RetryingAsyncCallbackSupplier<>(retryState, onAttemptFailure, + return new RetryingAsyncCallbackSupplier<>(retryState, onRetryableReadAttemptFailure(operationContext), CommandOperationHelper::shouldAttemptToRetryRead, callback -> { logRetryExecute(retryState, operationContext); asyncReadFunction.get(callback); @@ -299,11 +296,7 @@ static AsyncCallbackSupplier decorateReadWithRetriesAsync(final RetryStat static AsyncCallbackSupplier decorateWriteWithRetriesAsync(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier asyncWriteFunction) { - BinaryOperator onAttemptFailure = - (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> - CommandOperationHelper.onRetryableWriteAttemptFailure( - operationContext, previouslyChosenException, mostRecentAttemptException); - return new RetryingAsyncCallbackSupplier<>(retryState, onAttemptFailure, + return new RetryingAsyncCallbackSupplier<>(retryState, onRetryableWriteAttemptFailure(operationContext), CommandOperationHelper::shouldAttemptToRetryWrite, callback -> { logRetryExecute(retryState, operationContext); asyncWriteFunction.get(callback); diff --git a/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java index a9614d18788..3f47ba06f89 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java @@ -36,6 +36,7 @@ import org.bson.BsonDocument; import java.util.List; +import java.util.function.BinaryOperator; import java.util.function.Supplier; import static com.mongodb.assertions.Assertions.assertFalse; @@ -51,12 +52,11 @@ interface CommandCreator { BsonDocument create(ServerDescription serverDescription, ConnectionDescription connectionDescription); } - static Throwable onRetryableReadAttemptFailure( - final OperationContext operationContext, - @Nullable final Throwable previouslyChosenException, - final Throwable mostRecentAttemptException) { - operationContext.getServerDeprioritization().onAttemptFailure(mostRecentAttemptException); - return chooseRetryableReadException(previouslyChosenException, mostRecentAttemptException); + static BinaryOperator onRetryableReadAttemptFailure(final OperationContext operationContext) { + return (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> { + operationContext.getServerDeprioritization().onAttemptFailure(mostRecentAttemptException); + return chooseRetryableReadException(previouslyChosenException, mostRecentAttemptException); + }; } private static Throwable chooseRetryableReadException( @@ -71,12 +71,11 @@ private static Throwable chooseRetryableReadException( } } - static Throwable onRetryableWriteAttemptFailure( - final OperationContext operationContext, - @Nullable final Throwable previouslyChosenException, - final Throwable mostRecentAttemptException) { - operationContext.getServerDeprioritization().onAttemptFailure(mostRecentAttemptException); - return chooseRetryableWriteException(previouslyChosenException, mostRecentAttemptException); + static BinaryOperator onRetryableWriteAttemptFailure(final OperationContext operationContext) { + return (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> { + operationContext.getServerDeprioritization().onAttemptFailure(mostRecentAttemptException); + return chooseRetryableWriteException(previouslyChosenException, mostRecentAttemptException); + }; } private static Throwable chooseRetryableWriteException( diff --git a/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java b/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java index ff30807b2b8..fe58fb0bd75 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java +++ b/driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java @@ -51,7 +51,6 @@ import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.function.BinaryOperator; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -63,6 +62,7 @@ import static com.mongodb.internal.operation.AsyncOperationHelper.withAsyncSourceAndConnection; import static com.mongodb.internal.operation.CommandOperationHelper.addRetryableWriteErrorLabel; import static com.mongodb.internal.operation.CommandOperationHelper.logRetryExecute; +import static com.mongodb.internal.operation.CommandOperationHelper.onRetryableWriteAttemptFailure; import static com.mongodb.internal.operation.CommandOperationHelper.transformWriteException; import static com.mongodb.internal.operation.OperationHelper.LOGGER; import static com.mongodb.internal.operation.OperationHelper.isRetryableWrite; @@ -141,11 +141,7 @@ public Boolean getRetryWrites() { private Supplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier writeFunction) { - BinaryOperator onAttemptFailure = - (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> - CommandOperationHelper.onRetryableWriteAttemptFailure( - operationContext, previouslyChosenException, mostRecentAttemptException); - return new RetryingSyncSupplier<>(retryState, onAttemptFailure, + return new RetryingSyncSupplier<>(retryState, onRetryableWriteAttemptFailure(operationContext), this::shouldAttemptToRetryWrite, () -> { logRetryExecute(retryState, operationContext); return writeFunction.get(); @@ -154,11 +150,7 @@ private Supplier decorateWriteWithRetries(final RetryState retryState, fi private AsyncCallbackSupplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final AsyncCallbackSupplier writeFunction) { - BinaryOperator onAttemptFailure = - (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> - CommandOperationHelper.onRetryableWriteAttemptFailure( - operationContext, previouslyChosenException, mostRecentAttemptException); - return new RetryingAsyncCallbackSupplier<>(retryState, onAttemptFailure, + return new RetryingAsyncCallbackSupplier<>(retryState, onRetryableWriteAttemptFailure(operationContext), this::shouldAttemptToRetryWrite, callback -> { logRetryExecute(retryState, operationContext); writeFunction.get(callback); diff --git a/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java b/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java index 6f548f4b4d2..5610f84dd36 100644 --- a/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java +++ b/driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java @@ -41,7 +41,6 @@ import org.bson.codecs.Decoder; import java.util.function.BiFunction; -import java.util.function.BinaryOperator; import java.util.function.Function; import java.util.function.Supplier; @@ -52,6 +51,8 @@ import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE; import static com.mongodb.internal.operation.CommandOperationHelper.CommandCreator; import static com.mongodb.internal.operation.CommandOperationHelper.logRetryExecute; +import static com.mongodb.internal.operation.CommandOperationHelper.onRetryableReadAttemptFailure; +import static com.mongodb.internal.operation.CommandOperationHelper.onRetryableWriteAttemptFailure; import static com.mongodb.internal.operation.OperationHelper.ResourceSupplierInternalException; import static com.mongodb.internal.operation.OperationHelper.canRetryRead; import static com.mongodb.internal.operation.OperationHelper.canRetryWrite; @@ -275,11 +276,7 @@ static T createReadCommandAndExecute( static Supplier decorateWriteWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier writeFunction) { - BinaryOperator onAttemptFailure = - (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> - CommandOperationHelper.onRetryableWriteAttemptFailure( - operationContext, previouslyChosenException, mostRecentAttemptException); - return new RetryingSyncSupplier<>(retryState, onAttemptFailure, + return new RetryingSyncSupplier<>(retryState, onRetryableWriteAttemptFailure(operationContext), CommandOperationHelper::shouldAttemptToRetryWrite, () -> { logRetryExecute(retryState, operationContext); return writeFunction.get(); @@ -288,11 +285,7 @@ static Supplier decorateWriteWithRetries(final RetryState retryState, static Supplier decorateReadWithRetries(final RetryState retryState, final OperationContext operationContext, final Supplier readFunction) { - BinaryOperator onAttemptFailure = - (@Nullable Throwable previouslyChosenException, Throwable mostRecentAttemptException) -> - CommandOperationHelper.onRetryableReadAttemptFailure( - operationContext, previouslyChosenException, mostRecentAttemptException); - return new RetryingSyncSupplier<>(retryState, onAttemptFailure, + return new RetryingSyncSupplier<>(retryState, onRetryableReadAttemptFailure(operationContext), CommandOperationHelper::shouldAttemptToRetryRead, () -> { logRetryExecute(retryState, operationContext); return readFunction.get(); From cbb193889991633bf883ae5e6fc328e6833fc69c Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Wed, 8 May 2024 17:52:05 -0600 Subject: [PATCH 17/19] Link from `BaseClusterTest` back to `BaseClusterSpecification` JAVA-4254 --- .../unit/com/mongodb/internal/connection/BaseClusterTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java index f9b3dd6ff2d..641f814a6dd 100644 --- a/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java +++ b/driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterTest.java @@ -30,6 +30,9 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.mockito.Mockito.when; +/** + * @see BaseClusterSpecification + */ final class BaseClusterTest { @Test void selectServerToleratesWhenThereIsNoServerForTheSelectedAddress() { From 05f6679e29c9826bb4fa1d9c6965c0a2ff03a8f0 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Thu, 23 May 2024 07:59:28 -0600 Subject: [PATCH 18/19] Simplify `MinimumOperationCountServerSelector.select` JAVA-4254 --- .../selector/MinimumOperationCountServerSelector.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java b/driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java index 348b569aa9b..8acc5978c1f 100644 --- a/driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java +++ b/driver-core/src/main/com/mongodb/internal/selector/MinimumOperationCountServerSelector.java @@ -23,11 +23,11 @@ import com.mongodb.internal.connection.Server; import com.mongodb.selector.ServerSelector; +import java.util.Collections; import java.util.List; import static com.mongodb.assertions.Assertions.assertNotNull; import static java.util.Collections.emptyList; -import static java.util.Collections.singletonList; import static java.util.Comparator.comparingInt; /** @@ -51,12 +51,12 @@ public MinimumOperationCountServerSelector(final ServersSnapshot serversSnapshot @Override public List select(final ClusterDescription clusterDescription) { - ServerDescription selected = clusterDescription.getServerDescriptions() + return clusterDescription.getServerDescriptions() .stream() .min(comparingInt(serverDescription -> assertNotNull(serversSnapshot.getServer(serverDescription.getAddress())) .operationCount())) - .orElse(null); - return selected == null ? emptyList() : singletonList(selected); + .map(Collections::singletonList) + .orElse(emptyList()); } } From d55c44f691780f2f8c79296f213e6b2a91556012 Mon Sep 17 00:00:00 2001 From: Valentin Kovalenko Date: Fri, 24 May 2024 12:06:28 -0600 Subject: [PATCH 19/19] Extract `raceConditionPreFiltering` into a separate method and remove the two last lines of the corresponding comment, as they are not informative. JAVA-4254 --- .../internal/connection/BaseCluster.java | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java index d7443d39c1b..292822244b7 100644 --- a/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java +++ b/driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java @@ -340,25 +340,8 @@ private static ServerSelector getCompleteServerSelector( final ServerDeprioritization serverDeprioritization, final ServersSnapshot serversSnapshot, final ClusterSettings settings) { - // The set of `Server`s maintained by the `Cluster` is updated concurrently with `clusterDescription` being read. - // Additionally, that set of servers continues to be concurrently updated while `serverSelector` selects. - // This race condition means that we are not guaranteed to observe all the servers from `clusterDescription` - // among the `Server`s maintained by the `Cluster`. - // To deal with this race condition, we take `serversSnapshot` of that set of `Server`s - // (the snapshot itself does not have to be atomic) non-atomically with reading `clusterDescription` - // (this means, `serversSnapshot` and `clusterDescription` are not guaranteed to be consistent with each other), - // and do pre-filtering to make sure that the only `ServerDescription`s we may select, - // are of those `Server`s that are known to both `clusterDescription` and `serversSnapshot`. - // This way we are guaranteed to successfully get `Server`s from `serversSnapshot` based on the selected `ServerDescription`s. - // - // The pre-filtering we do to deal with the race condition described above is achieved by this `ServerSelector`. - ServerSelector raceConditionPreFiltering = clusterDescriptionPotentiallyInconsistentWithServerSnapshot -> - clusterDescriptionPotentiallyInconsistentWithServerSnapshot.getServerDescriptions() - .stream() - .filter(serverDescription -> serversSnapshot.containsServer(serverDescription.getAddress())) - .collect(toList()); List selectors = Stream.of( - raceConditionPreFiltering, + getRaceConditionPreFilteringSelector(serversSnapshot), serverSelector, serverDeprioritization.getServerSelector(), settings.getServerSelector(), // may be null @@ -369,6 +352,22 @@ private static ServerSelector getCompleteServerSelector( return new CompositeServerSelector(selectors); } + private static ServerSelector getRaceConditionPreFilteringSelector(final ServersSnapshot serversSnapshot) { + // The set of `Server`s maintained by the `Cluster` is updated concurrently with `clusterDescription` being read. + // Additionally, that set of servers continues to be concurrently updated while `serverSelector` selects. + // This race condition means that we are not guaranteed to observe all the servers from `clusterDescription` + // among the `Server`s maintained by the `Cluster`. + // To deal with this race condition, we take `serversSnapshot` of that set of `Server`s + // (the snapshot itself does not have to be atomic) non-atomically with reading `clusterDescription` + // (this means, `serversSnapshot` and `clusterDescription` are not guaranteed to be consistent with each other), + // and do pre-filtering to make sure that the only `ServerDescription`s we may select, + // are of those `Server`s that are known to both `clusterDescription` and `serversSnapshot`. + return clusterDescription -> clusterDescription.getServerDescriptions() + .stream() + .filter(serverDescription -> serversSnapshot.containsServer(serverDescription.getAddress())) + .collect(toList()); + } + protected ClusterableServer createServer(final ServerAddress serverAddress) { return serverFactory.create(this, serverAddress); }