CSHARP-3671: Better wait queue timeout errors for load balanced clusters. #579

DmitryLukyanov · 2021-07-14T17:20:12Z

No description provided.

DmitryLukyanov · 2021-07-15T00:32:48Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ExclusiveConnectionPool.Helpers.cs

+            var numberOfCheckOutsForCursor = pool._checkedOutTracker.GetCheckedOutNumber(CheckedOutReason.Cursor);
+            var numberOfCheckOutsForTransaction = pool._checkedOutTracker.GetCheckedOutNumber(CheckedOutReason.Transaction);
+
+            var message = numberOfCheckOutsForCursor == 0 && numberOfCheckOutsForTransaction == 0


this logic is copied from java, if we have no checkouts for transaction or cursor, we use old variant of exception message

also, this logic can be simplified if in the current master version we will leave only one timeout exception message (we have two that are a bit different), it looks like a single timeout error message is how java behaves.
But I will be ok with the current way also. cc: @rstam

DmitryLukyanov · 2021-07-15T00:36:32Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

@@ -104,14 +104,14 @@ public override IAsyncCursor<TResult> Aggregate<TResult>(IClientSessionHandle se
                var forkedSession = session.Fork();
                var deferredCursor = new DeferredAsyncCursor<TResult>(
                    () => forkedSession.Dispose(),
-                    ct => ExecuteReadOperation(forkedSession, findOperation, ReadPreference.Primary, ct),
-                    ct => ExecuteReadOperationAsync(forkedSession, findOperation, ReadPreference.Primary, ct));
+                    ct => ExecuteReadOperation(forkedSession, findOperation, ReadPreference.Primary, doesInitiateCursor: true, ct),


in theory we can avoid such changes via analyzing an operation type (which is like IReadOperation<IAsyncCursor<TResult>>), but I didn't find a way to do it without reflection which can reduce performance

DmitryLukyanov · 2021-07-15T00:37:14Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedListCollectionsOperation.cs

@@ -63,7 +63,7 @@ public async Task<OperationResult> ExecuteAsync(CancellationToken cancellationTo
                    ? await _database.ListCollectionsAsync(_options, cancellationToken)
                    : await _database.ListCollectionsAsync(_session, _options, cancellationToken);

-                _ = cursor.ToListAsync(cancellationToken);
+                _ = await cursor.ToListAsync(cancellationToken);


this and below are changes from a different PR

…ers.

rstam

Please re-request my review when you've added the new commit you said you were working on.

DmitryLukyanov · 2021-07-21T16:44:31Z

The latest 2 commits contains simplifying of the logic in this PR:

(Simplifying.: the second from the end): de9ab50
EG: https://evergreen.mongodb.com/version/60f843552fbabe15604d13da
Revert most of the changes in this PR, but didn't change the pinning logic itself. So, I changed only how we set a reason.
(Simplifying (v2) : the last commit): ee358f6. This PR continues simplifying started in the first commit. With this change we don't need to replace channel/channelSource in the retry logic anymore. This change assumes that any operation won't require acquiring connection more than initial once. So, for the case when we only create a cursor, the actual channel pinning will happen only before creating cursor itself (previously I pinned connection right after creating)
EG: https://evergreen.mongodb.com/version/60f84c70c9ec444dca52e795

rstam

I like this approach a lot. It makes sense.

Don't be dismayed by the number of requested changes. They are almost all requests to rename something.

Requested changes prototyped here:

https://github.com/rstam/mongo-csharp-driver/tree/csharp3671-0722

rstam · 2021-07-23T01:07:37Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ExclusiveConnectionPool.cs

@@ -29,6 +29,7 @@ namespace MongoDB.Driver.Core.ConnectionPools
    internal sealed partial class ExclusiveConnectionPool : IConnectionPool
    {
        // fields
+        private readonly CheckedOutTracker _checkedOutTracker;


private readonly CheckoutReasonCounter _checkoutReasonCounter;

rstam · 2021-07-23T01:07:50Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ExclusiveConnectionPool.cs

@@ -71,6 +72,7 @@ internal sealed partial class ExclusiveConnectionPool : IConnectionPool
            _connectionFactory = Ensure.IsNotNull(connectionFactory, nameof(connectionFactory));
            Ensure.IsNotNull(eventSubscriber, nameof(eventSubscriber));

+            _checkedOutTracker = new CheckedOutTracker();


_checkoutReasonCounter = new CheckoutReasonCounter();

rstam · 2021-07-23T01:08:07Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ExclusiveConnectionPool.cs

@@ -385,6 +387,8 @@ private void ReleaseConnection(PooledConnection connection)
                _checkedInConnectionEventHandler(new ConnectionPoolCheckedInConnectionEvent(connection.ConnectionId, TimeSpan.Zero, EventContext.OperationId));
            }

+            _checkedOutTracker.CheckInIfNotNull(connection.CheckedOutReason);


_checkoutReasonCounter.Decrement(connection.CheckoutReason);

src/MongoDB.Driver.Core/Core/ConnectionPools/ExclusiveConnectionPool.Helpers.cs

rstam · 2021-07-23T01:09:37Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ExclusiveConnectionPool.Helpers.cs

@@ -39,6 +39,23 @@ private static class State
            public const int Disposed = 2;
        }

+        private static Exception CreateEffectiveTimeoutException(string defaultExceptionMessage, ExclusiveConnectionPool pool, Stopwatch stopwatch)


This should be an instance method of the ExclusiveConnectionPool:

// private methods private Exception CreateTimeoutException(Stopwatch stopwatch, string message) { var checkoutsForCursorCount = _checkoutReasonCounter.GetCheckoutsCount(CheckoutReason.Cursor); var checkoutsForTransactionCount = _checkoutReasonCounter.GetCheckoutsCount(CheckoutReason.Transaction); // only use the expanded message format when connected to a load balancer if (checkoutsForCursorCount != 0 || checkoutsForTransactionCount != 0) { var maxPoolSize = _settings.MaxConnections; var availableConnectionsCount = AvailableCount; var checkoutsCount = maxPoolSize - availableConnectionsCount; var checkoutsForOtherCount = checkoutsCount - checkoutsForCursorCount - checkoutsForTransactionCount; message = $"Timed out after {stopwatch.ElapsedMilliseconds}ms waiting for a connection from the connection pool. " + $"maxPoolSize: {maxPoolSize}, " + $"connections in use by cursors: {checkoutsForCursorCount}, " + $"connections in use by transactions: {checkoutsForTransactionCount}, " + $"connections in use by other operations: {checkoutsForOtherCount}."; } return new TimeoutException(message); }

Also... move to after line 33 (this is not a nested class).

Note: yet another reason why this should not be a partial class. This is an instance method of the ExclusiveConnectionPool class. According to the split of this class into two files (which I disagree with) this instance method should be in the ExclusiveConnectionPool.cs file, BUT... it is only used in this file.

So where should it go?

Yet another example of why this is not how partial classes should be used.

rstam · 2021-07-23T01:28:46Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

+            }
+        }
+
+        public void CheckInIfNotNull(CheckedOutReason? reason)


public void Decrement(CheckoutReason? reason)

rstam · 2021-07-23T01:28:57Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

+                        // should not be reached
+                        throw new InvalidOperationException($"Unsupported checked out reason {reason}.");
+                }
+            }


Replace 71-88 with:

switch (reason) { case null: break; case CheckoutReason.Cursor: Interlocked.Decrement(ref _cursorCheckoutsCount); break; case CheckoutReason.Transaction: Interlocked.Decrement(ref _transactionCheckoutsCount); break; default: throw new InvalidOperationException($"Invalid checkout reason {reason}."); }

src/MongoDB.Driver.Core/Core/ChannelPinningHelper.cs

rstam · 2021-07-23T01:29:56Z

src/MongoDB.Driver.Core/Core/ChannelPinningHelper.cs


+                // pinning channel


Do you mean?

// pin to channel

I meant that this is pinning implementation(to cursor) requested in the ticket: Update connection pinning logic for LB mode, I will just remove this comment

rstam · 2021-07-23T01:30:23Z

src/MongoDB.Driver.Core/Core/ChannelPinningHelper.cs

                {
-                    session.CurrentTransaction.PinChannel(channel.Fork());
-                    session.CurrentTransaction.PinnedServer = server;
+                    trackedConnection.SetPinningCheckoutReasonIfNotAlreadySet(CheckedOutReason.Transaction);


if (channel.Connection is ICheckoutReasonTracker checkoutReasonTracker) { checkoutReasonTracker.SetCheckoutReasonIfNotAlreadySet(CheckoutReason.Transaction); }

DmitryLukyanov

ready for review

DmitryLukyanov · 2021-07-23T10:10:33Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

+
+namespace MongoDB.Driver.Core.ConnectionPools
+{
+    internal enum CheckedOutReason


previously it shown a difference between Event*ing* and Event*ed*, but now it's useless anymore, done

DmitryLukyanov · 2021-07-23T10:51:28Z

src/MongoDB.Driver.Core/Core/ChannelPinningHelper.cs


+                // pinning channel


I meant that this is pinning implementation(to cursor) requested in the ticket: Update connection pinning logic for LB mode, I will just remove this comment

src/MongoDB.Driver.Core/Core/ChannelPinningHelper.cs

DmitryLukyanov · 2021-07-23T10:52:16Z

src/MongoDB.Driver.Core/Core/ChannelPinningHelper.cs

                {
-                    session.CurrentTransaction.PinChannel(channel.Fork());
-                    session.CurrentTransaction.PinnedServer = server;
+                    trackedConnection.SetPinningCheckoutReasonIfNotAlreadySet(CheckedOutReason.Transaction);


DmitryLukyanov · 2021-07-23T10:54:07Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

+                    break;
+                default:
+                    // should not be reached
+                    throw new InvalidOperationException($"Unsupported checked out reason {reason}.");


DmitryLukyanov · 2021-07-23T10:54:51Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

@@ -0,0 +1,91 @@
+/*Copyright 2021 - present MongoDB Inc.


DmitryLukyanov · 2021-07-23T10:55:54Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

+{
+    internal enum CheckedOutReason
+    {
+        NotSet,


yep, this was a bug, originally I set reason for any checkout operations, now it's not and I missed this case

DmitryLukyanov · 2021-07-23T10:58:41Z

src/MongoDB.Driver.Core/Core/ConnectionPools/ITrackedPinningReason.cs

+        Transaction
+    }
+
+    internal interface ITrackedPinningReason


rstam

LGTM (looks great to me)

…ers. (mongodb#579)

…ers. (mongodb#579) CSHARP-3671: Better wait queue timeout errors for load balanced clusters.

jyemin requested a review from rstam July 14, 2021 17:28

DmitryLukyanov force-pushed the csharp3671 branch 2 times, most recently from f7f627e to 0f5b393 Compare July 15, 2021 00:30

DmitryLukyanov commented Jul 15, 2021

View reviewed changes

CSHARP-3671: Better wait queue timeout errors for load balanced clust…

a610696

…ers.

DmitryLukyanov force-pushed the csharp3671 branch from 0f5b393 to a610696 Compare July 15, 2021 00:41

rstam requested changes Jul 20, 2021

View reviewed changes

CSHARP-3671: Simplifying.

de9ab50

DmitryLukyanov requested a review from rstam July 21, 2021 16:42

CSHARP-3671: Simplifying (v2).

0805ff0

DmitryLukyanov force-pushed the csharp3671 branch from ee358f6 to 0805ff0 Compare July 21, 2021 17:14

rstam requested changes Jul 23, 2021

View reviewed changes

CSHARP-3671: Code review.

d3d0960

DmitryLukyanov commented Jul 23, 2021

View reviewed changes

DmitryLukyanov requested a review from rstam July 23, 2021 17:10

rstam approved these changes Jul 23, 2021

View reviewed changes

DmitryLukyanov merged commit 882e798 into mongodb:master Jul 23, 2021

BorisDog pushed a commit to BorisDog/mongo-csharp-driver that referenced this pull request Jul 26, 2021

CSHARP-3671: Better wait queue timeout errors for load balanced clust…

b12ac63

…ers. (mongodb#579)

BorisDog pushed a commit to BorisDog/mongo-csharp-driver that referenced this pull request Aug 4, 2021

CSHARP-3671: Better wait queue timeout errors for load balanced clust…

a553ffb

…ers. (mongodb#579) CSHARP-3671: Better wait queue timeout errors for load balanced clusters.

CSHARP-3671: Better wait queue timeout errors for load balanced clusters. #579

CSHARP-3671: Better wait queue timeout errors for load balanced clusters. #579

Uh oh!

Conversation

DmitryLukyanov commented Jul 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov commented Jul 21, 2021

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DmitryLukyanov Jul 15, 2021 •

edited

Loading

rstam Jul 23, 2021 •

edited

Loading

DmitryLukyanov left a comment •

edited

Loading