CSHARP-5560: CSOT: Refactor IOperationExecutor to use operation context #1676

sanych-sun · 2025-04-24T00:08:36Z

No description provided.

adelinowona · 2025-04-29T20:10:34Z

src/MongoDB.Driver/OperationExecutor.cs

        {
-            return operation.Execute(binding, cancellationToken);
+            bool isOwnSession = session == null;


nit: use var?

here and other methods in this file

adelinowona · 2025-04-29T20:12:24Z

src/MongoDB.Driver/OperationExecutor.cs

        {
-            return _client.StartImplicitSessionAsync(cancellationToken);
+            var options = new ClientSessionOptions { CausalConsistency = false, Snapshot = false };
+            ICoreSessionHandle coreSession = _client.GetClusterInternal().StartSession(options.ToCore(isImplicit: true));


nit: use var?

adelinowona · 2025-04-29T20:22:09Z

src/MongoDB.Driver/ReadOperationOptions.cs

+
+namespace MongoDB.Driver
+{
+    internal record class ReadOperationOptions(ReadPreference ExplicitReadPreference = null, ReadPreference DefaultReadPreference = null);


no need for specifying class? I believe record is equivalent to record class

I'm OK with removing the class here. I've left it only to stress on the fact that it should be class, not struct. As changing to struct might affect its functionality (because of the copy-by-value nature of structs).

yeah let's remove the class, record is by default a class and if someone tries to change it to struct then we can address it then in the PR containing that change.

adelinowona · 2025-04-29T22:02:01Z

src/MongoDB.Driver/MongoCollectionImpl.cs

-            return UsingImplicitSession(session => BulkWrite(session, requests, options, cancellationToken), cancellationToken);
+            using var session = OperationExecutor.StartImplicitSession(cancellationToken);
+            return BulkWrite(session, requests, options, cancellationToken);


I noticed that for some of the other methods above (like AggregateToCollection) you did not use OperationExecutor to create the implicit session and just used the existing UsingImplicitSession mechanism. Should we update all those use cases as well? or am I missing something?

The idea was to hide the implicit session creation inside of the OperationExecutor. But for some corner cases (like Aggregate method) we should have access to the created session, so sometimes we have to create the implicit session from outside.

Thank you for reporting this. I've converted rest of the methods and removed UsingImplicitSession.

adelinowona

Looks good! left some minor comments

src/MongoDB.Driver/AggregateHelper.cs

src/MongoDB.Driver/MongoClient.cs

src/MongoDB.Driver/OperationExecutor.cs

src/MongoDB.Driver/MongoCollectionImpl.cs

src/MongoDB.Driver/MongoDatabase.cs

tests/MongoDB.Driver.Tests/ReadOperationOptionsTests.cs

tests/MongoDB.Driver.Tests/OperationExecutorTests.cs

BorisDog

partial review: minor comments so far.

BorisDog · 2025-05-23T01:57:35Z

src/MongoDB.Driver/MongoCollectionImpl.cs

@@ -58,6 +58,8 @@ private MongoCollectionImpl(IMongoDatabase database, CollectionNamespace collect
            _documentSerializer = Ensure.IsNotNull(documentSerializer, nameof(documentSerializer));

            _messageEncoderSettings = GetMessageEncoderSettings();
+            _readOperationOptions = new ReadOperationOptions(DefaultReadPreference: _settings.ReadPreference);
+            _writeOperationOptions = new WriteOperationOptions();


minor:

_readOperationOptions = new() _writeOperationOptions = new(...)

BorisDog · 2025-05-23T02:33:26Z

src/MongoDB.Driver/MongoCollectionImpl.cs

+                Ensure.IsNotNullOrEmpty(name, nameof(name));
+                if (name == "*")
+                {
+                    throw new ArgumentException("Cannot specify '*' for the index name. Use DropAllAsync to drop all indexes.", "name");


minor: mistake in the original code, should be DropAll and DropAllAsync.

BorisDog · 2025-05-23T18:11:13Z

src/MongoDB.Driver/MongoDatabase.cs

@@ -50,6 +51,9 @@ public MongoDatabase(IMongoClient client, DatabaseNamespace databaseNamespace, M
            _settings = Ensure.IsNotNull(settings, nameof(settings)).Freeze();
            _cluster = Ensure.IsNotNull(cluster, nameof(cluster));
            _operationExecutor = Ensure.IsNotNull(operationExecutor, nameof(operationExecutor));
+
+            _readOperationOptions = new ReadOperationOptions(DefaultReadPreference: _settings.ReadPreference);
+            _writeOperationOptions = new WriteOperationOptions();


minor: new(...) ?

BorisDog · 2025-05-23T20:22:23Z

src/MongoDB.Driver/OperationExecutor.cs

+
+        private IReadWriteBindingHandle CreateReadWriteBinding(IClientSessionHandle session)
+        {
+            // TODO: CreateReadWriteBinding from MongoClient did not used ChannelPinningHelper, double-check if it's OK to start using it


Need to address this TODO.

BorisDog · 2025-05-23T20:24:47Z

src/MongoDB.Driver/MongoClient.cs

@@ -100,7 +104,7 @@ public MongoClient(string connectionString)
        internal MongoClient(IOperationExecutor operationExecutor, MongoClientSettings settings)
            : this(settings)
        {
-            _operationExecutor = operationExecutor;
+            _operationExecutor = new OperationExecutorWrapper(operationExecutor);


Consider using the Fork approach.

@BorisDog what is the Fork approach?

BorisDog · 2025-05-23T20:36:07Z

tests/MongoDB.Driver.Tests/OperationExecutorTests.cs

+    {
+        [Theory]
+        [ParameterAttributeData]
+        public async Task StartImplicitSession_should_call_cluster([Values(true, false)]bool isAsync)


minor: _cluster_StartSession

adelinowona · 2025-05-27T18:54:15Z

src/MongoDB.Driver/WriteOperationOptions.cs

+
+namespace MongoDB.Driver
+{
+    internal record class WriteOperationOptions();


remove class here too

sanych-sun requested a review from a team as a code owner April 24, 2025 00:08

sanych-sun requested review from BorisDog and adelinowona and removed request for a team April 24, 2025 00:08

sanych-sun added the maintenance label Apr 24, 2025

adelinowona reviewed Apr 29, 2025

View reviewed changes

adelinowona requested changes Apr 29, 2025

View reviewed changes

BorisDog requested changes Apr 29, 2025

View reviewed changes

sanych-sun requested review from BorisDog and adelinowona May 21, 2025 00:40

Oleksandr Poliakov added 7 commits May 21, 2025 10:45

CSHARP-5560: CSOT: Refactor IOperationExecutor to use operation context

d459c00

pr

bbb2213

PR

909a694

Adjust .editorconfig to prefer var

9cf13c7

PR

9d8dd10

PR

e57ae06

Fix broken tests

f187294

sanych-sun force-pushed the csharp5560 branch from f52fb37 to f187294 Compare May 21, 2025 17:52

BorisDog requested changes May 23, 2025

View reviewed changes

BorisDog reviewed May 23, 2025

View reviewed changes

adelinowona requested changes May 27, 2025

View reviewed changes

CSHARP-5560: CSOT: Refactor IOperationExecutor to use operation context #1676

Are you sure you want to change the base?

CSHARP-5560: CSOT: Refactor IOperationExecutor to use operation context #1676

Uh oh!

Conversation

sanych-sun commented Apr 24, 2025

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!

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!

adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BorisDog 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!

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!

Uh oh!