You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/03/13 08:52:43 UTC

[GitHub] [tinkerpop] heljoyLiu opened a new pull request #1263: dotnet: add session mode connection

heljoyLiu opened a new pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] spmallette commented on a change in pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r394600294
 
 

 ##########
 File path: gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs
 ##########
 @@ -235,17 +243,52 @@ private void NotifyAboutConnectionFailure(Exception exception)
 
         private async Task SendMessageAsync(RequestMessage message)
         {
+            if (_sessionEnabled)
+            {
+                message = RebuildSessionMessage(message);
+            }
             var graphsonMsg = _graphSONWriter.WriteObject(message);
             var serializedMsg = _messageSerializer.SerializeMessage(graphsonMsg);
             await _webSocketConnection.SendMessageAsync(serializedMsg).ConfigureAwait(false);
         }
 
+        private RequestMessage RebuildSessionMessage(RequestMessage message)
+        {
+            if (message.Processor == Tokens.OpsAuthentication)
+            {
+                return message;
+            }
+
+            var msgBuilder = RequestMessage.Build(message.Operation)
+              .OverrideRequestId(message.RequestId).Processor(Tokens.ProcessorSession);
+            foreach(var kv in message.Arguments)
+            {
+                msgBuilder.AddArgument(kv.Key, kv.Value);
+            }
+            msgBuilder.AddArgument(Tokens.ArgsSession, _sessionId);
+            return msgBuilder.Create();
+        }
+
         public async Task CloseAsync()
         {
             Interlocked.Exchange(ref _connectionState, Closed);
+
+            if (_sessionEnabled)
+            {
+                await CloseSession().ConfigureAwait(false);
+            }
             await _webSocketConnection.CloseAsync().ConfigureAwait(false);
         }
 
+        private async Task CloseSession()
+        {
+            // build a request to close this session, 'gremlin' in args as tips and not be executed actually
+            var msg = RequestMessage.Build(Tokens.OpsClose).Processor(Tokens.ProcessorSession)
+              .AddArgument(Tokens.ArgsGremlin, "session.close()").Create();
 
 Review comment:
   It seems we accepted this pattern here for Javascript:
   
   https://github.com/apache/tinkerpop/commit/1751d25ad4b664a70f254ac2822bdae729014256#diff-9ec1348d608a722cece8543e6db87a23R225
   
   I think I overlooked that in review. I don't think we should include an arg just for the sake of it. I would also go a step further and say that we probably shouldn't bother to add support for the "close" message at all.  We just deprecated the "close" message in a recent batch of changes and no longer send it at all in the Java driver in 3.5.0:
   
   https://github.com/apache/tinkerpop/blob/0a7769eea651ab0e72d3a6a0bd424fbb1197bd47/docs/src/upgrade/release-3.5.x.asciidoc#session-close
   
   I don't think it's worth adding now, unless we can come up with a good reason for doing so. The only argument I can think of is that it would make the drivers backward compatible with sessions starting at 3.4.7, but even then the "close" message is a bit flawed on the server in those earlier versions:
   
   https://issues.apache.org/jira/browse/TINKERPOP-2336
   
   so, is there real value to adding the code on the client? If the answer to that is "yes" with some reasoning then we'd need a second PR to remove the code on the "master" branch to match the Java Driver. Thoughts?
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] spmallette commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603392326
 
 
   I tried to merge this through to `master` and the test in this PR fails. This exception gets raised:
   
   https://github.com/apache/tinkerpop/blob/c3092069f0961759036b55c5f874a4c2fd1d4978/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java#L194
   
   I guess that .NET doesn't end up using the same channel for sessions the way that the Java driver does. I think we'd want this change to `3.4-dev` to include a fix that enables that.  
   
   @FlorianHockmann do you have any other insight on this one?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on a change in pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r401628899
 
 

 ##########
 File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/GremlinClientTests.cs
 ##########
 @@ -285,12 +285,22 @@ public async Task ShouldSaveVariableBetweenRequestsInSession()
             var gremlinServer = new GremlinServer(TestHost, TestPort);
             using (var gremlinClient = new GremlinClient(gremlinServer, sessionId: Guid.NewGuid().ToString()))
             {
-
                 await gremlinClient.SubmitWithSingleResultAsync<int>("x = 1");
 
                 var response = await gremlinClient.SubmitWithSingleResultAsync<int>("x + 2");
                 Assert.Equal(3, response);
             }
         }
+
+        [Fact]
+        public void ShouldThrowOnExecutionOfMultiConnectionInPool()
 
 Review comment:
   ok, it's UnitTest, thanks

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] spmallette commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603766780
 
 
   > it only failed when you had it merged into master locally? Any idea why it doesn't already fail on 3.4-dev / in this PR?
   
   Yes - it's because of: https://issues.apache.org/jira/browse/TINKERPOP-2336
   
   > The Java driver only allows a session to be used by a single channel and therefore a single connection?
   
   I don't think the Java driver does anything fancy to allow that. It just configured a connection pool of size 1 and doesn't allow it to grow. That creates a single Netty `Channel` for the session to communicate over.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603827895
 
 
   > and any reconfigure will be ignore about pool size
   
   I would actually throw an exception in that case so users know that their configuration is invalid for sessions and connect be applied. Other than that, this sounds like a good and easy approach to me.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on a change in pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r401629785
 
 

 ##########
 File path: gremlin-dotnet/src/Gremlin.Net/Driver/GremlinClient.cs
 ##########
 @@ -69,6 +69,21 @@ public class GremlinClient : IGremlinClient
             var writer = graphSONWriter ?? new GraphSON3Writer();
             var connectionFactory = new ConnectionFactory(gremlinServer, reader, writer, mimeType ?? DefaultMimeType,
                 webSocketConfiguration, sessionId);
+
+            // make sure one connection in pool as session mode
+            if (!String.IsNullOrEmpty(sessionId))
+            {
+                if (connectionPoolSettings != null)
+                {
+                    if (connectionPoolSettings.PoolSize != 1)
+                        throw new ArgumentOutOfRangeException(nameof(connectionPoolSettings), "Session Client PoolSize must be 1!");
 
 Review comment:
   ok, session mode should be better

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on a change in pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r394831049
 
 

 ##########
 File path: gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs
 ##########
 @@ -235,17 +243,52 @@ private void NotifyAboutConnectionFailure(Exception exception)
 
         private async Task SendMessageAsync(RequestMessage message)
         {
+            if (_sessionEnabled)
+            {
+                message = RebuildSessionMessage(message);
+            }
             var graphsonMsg = _graphSONWriter.WriteObject(message);
             var serializedMsg = _messageSerializer.SerializeMessage(graphsonMsg);
             await _webSocketConnection.SendMessageAsync(serializedMsg).ConfigureAwait(false);
         }
 
+        private RequestMessage RebuildSessionMessage(RequestMessage message)
+        {
+            if (message.Processor == Tokens.OpsAuthentication)
+            {
+                return message;
+            }
+
+            var msgBuilder = RequestMessage.Build(message.Operation)
+              .OverrideRequestId(message.RequestId).Processor(Tokens.ProcessorSession);
+            foreach(var kv in message.Arguments)
+            {
+                msgBuilder.AddArgument(kv.Key, kv.Value);
+            }
+            msgBuilder.AddArgument(Tokens.ArgsSession, _sessionId);
+            return msgBuilder.Create();
+        }
+
         public async Task CloseAsync()
         {
             Interlocked.Exchange(ref _connectionState, Closed);
+
+            if (_sessionEnabled)
+            {
+                await CloseSession().ConfigureAwait(false);
+            }
             await _webSocketConnection.CloseAsync().ConfigureAwait(false);
         }
 
+        private async Task CloseSession()
+        {
+            // build a request to close this session, 'gremlin' in args as tips and not be executed actually
+            var msg = RequestMessage.Build(Tokens.OpsClose).Processor(Tokens.ProcessorSession)
+              .AddArgument(Tokens.ArgsGremlin, "session.close()").Create();
 
 Review comment:
   I don't notice that sorry, and a good job to bound to client, great
   I will remove 'close'  in this PR, 
   and another PR for javascript is going on. thanks

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-607270583
 
 
   LGTM again, so my VOTE is +1 again.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-600405859
 
 
   > Could you please try merging in your branch for `3.3-dev` into this feature branch and resolve the merge conflicts? That would make it a lot easier to merge your two pull requests as the contributor who does that otherwise has to resolve the merge conflicts then.
   > If you don't have a specific reason for this change to go into `3.3-dev`, then we can also only merge this PR into `3.4-dev` and close the PR for `3.3-dev`. We usually only apply bug fixes to the `3.3-dev` branch by now and not new features. So, this contribution doesn't have to go into `3.3-dev` from my side.
   
   It's fine to merge `3.4-dev` only, and this PR is for `3.4-dev` without conflicts.
   so, I will close [PR](https://github.com/apache/tinkerpop/pull/1257) for `3.3-dev`, thanks

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] spmallette commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-607294822
 
 
   The tests now pass on `master`, which solves my original issues with this PR.  In that sense, VOTE +1.
   
   There remains a bit of question about documentation, but perhaps we could just have unified documentation for all GLVs that picked up this feature after #1264 is merged. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-600206341
 
 
   Could you please try merging in your branch for `3.3-dev` into this feature branch and resolve the merge conflicts? That would make it a lot easier to merge your two pull requests as the contributor who does that otherwise has to resolve the merge conflicts then.
   If you don't have a specific reason for this change to go into `3.3-dev`, then we can also only merge this PR into `3.4-dev` and close the PR for `3.3-dev`. We usually only apply bug fixes to the `3.3-dev` branch by now and not new features. So, this contribution doesn't have to go into `3.3-dev` from my side.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] spmallette merged pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-598619973
 
 
   hi, @FlorianHockmann 
   a similar RP for `3.4-dev` about session in `gremlin-dotnet`,  please take a look when free.
   by the way, it's a big upgrade `3.4-dev`. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-607205156
 
 
   > > and any reconfigure will be ignore about pool size
   > 
   > I would actually throw an exception in that case so users know that their configuration is invalid for sessions and connect be applied. Other than that, this sounds like a good and easy approach to me.
   
   en, it's great. 
   and more question about connection, would  .Net SDK disconnect it when idle and reconnect again when busy ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603744381
 
 
   > I tried to merge this through to master and the test in this PR fails.
   
   It only failed when you had it merged into `master` locally? Any idea why it doesn't already fail on `3.4-dev` / in this PR?
   
   > I guess that .NET doesn't end up using the same channel for sessions the way that the Java driver does.
   
   The Java driver only allows a session to be used by a single channel and therefore a single connection?
   That increases the complexity for this feature for Gremlin.NET as it now needs to assign one connection for each session. If this gets too complex, then we will need to make the decision of whether we actually want to support this feature in .NET, but maybe it's also actually easier to implement and maintain then I'm thinking right now.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r401582778
 
 

 ##########
 File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/GremlinClientTests.cs
 ##########
 @@ -285,12 +285,22 @@ public async Task ShouldSaveVariableBetweenRequestsInSession()
             var gremlinServer = new GremlinServer(TestHost, TestPort);
             using (var gremlinClient = new GremlinClient(gremlinServer, sessionId: Guid.NewGuid().ToString()))
             {
-
                 await gremlinClient.SubmitWithSingleResultAsync<int>("x = 1");
 
                 var response = await gremlinClient.SubmitWithSingleResultAsync<int>("x + 2");
                 Assert.Equal(3, response);
             }
         }
+
+        [Fact]
+        public void ShouldThrowOnExecutionOfMultiConnectionInPool()
 
 Review comment:
   The test looks good in general, but it is not really an integration test as it works without any external dependencies like a Gremlin Server instance. Could you therefore please move it into the `Gremlin.Net.UnitTest` project? You can create a new class `GremlinClientTests` in the `Driver` directory there as there probably are no unit tests yet for this class.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603781189
 
 
   > > so, configure for connection pool should be set in .Net as 4 conns in pool by default, is't ?
   > 
   > I don't think it's quite as easy as a change of the default, because someone could reconfigure it back to something other than 1. The Java driver has a way of preventing the user from doing that so that sessions can't get messed up. .NET would need something similar IMO.
   
   en,  add a commit to set pool size to 1 in .NET during creating SessionClient, like Java driver. and any reconfigure will be ignore about pool size

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603592967
 
 
   > I tried to merge this through to `master` and the test in this PR fails. This exception gets raised:
   > 
   > https://github.com/apache/tinkerpop/blob/c3092069f0961759036b55c5f874a4c2fd1d4978/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java#L194
   > 
   > I guess that .NET doesn't end up using the same channel for sessions the way that the Java driver does. I think we'd want this change to `3.4-dev` to include a fix that enables that.
   > 
   > @FlorianHockmann do you have any other insight on this one?
   
   As bound the client channel to a session,  I think only one connection is allowed in SessionClient. I will verify that.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] heljoyLiu commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603769338
 
 
   
   > 
   > I don't think the Java driver does anything fancy to allow that. It just configured a connection pool of size 1 and doesn't allow it to grow. That creates a single Netty `Channel` for the session to communicate over.
   
   so, configure for connection pool should be set in .Net as 4 conns in pool by default, is't ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r401584750
 
 

 ##########
 File path: gremlin-dotnet/src/Gremlin.Net/Driver/GremlinClient.cs
 ##########
 @@ -69,6 +69,21 @@ public class GremlinClient : IGremlinClient
             var writer = graphSONWriter ?? new GraphSON3Writer();
             var connectionFactory = new ConnectionFactory(gremlinServer, reader, writer, mimeType ?? DefaultMimeType,
                 webSocketConfiguration, sessionId);
+
+            // make sure one connection in pool as session mode
+            if (!String.IsNullOrEmpty(sessionId))
+            {
+                if (connectionPoolSettings != null)
+                {
+                    if (connectionPoolSettings.PoolSize != 1)
+                        throw new ArgumentOutOfRangeException(nameof(connectionPoolSettings), "Session Client PoolSize must be 1!");
 
 Review comment:
   I am not sure whether every user will understand what `Session Client` means as we don't use that term any where else I think. Could you maybe reword it to something like: _Only a pool size of 1 is supported in session mode._?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [tinkerpop] spmallette commented on issue #1263: dotnet: add session mode connection

Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#issuecomment-603773249
 
 
   > so, configure for connection pool should be set in .Net as 4 conns in pool by default, is't ?
   
   I don't think it's quite as easy as a change of the default, because someone could reconfigure it back to something other than 1. The Java driver has a way of preventing the user from doing that so that sessions can't get messed up. .NET would need something similar IMO. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services