You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by "heljoyLiu (GitHub)" <gi...@apache.org> on 2020/03/04 13:34:44 UTC
[GitHub] [tinkerpop] heljoyLiu opened pull request #1257: dotnet: add
session connection
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1257:
dotnet: add session connection
Posted by "FlorianHockmann (GitHub)" <gi...@apache.org>.
(nitpick) **s**tring (it's just the usual C# code style)
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1257:
dotnet: add session connection
Posted by "FlorianHockmann (GitHub)" <gi...@apache.org>.
Any reason why you didn't simply call `SendAsync` here? I think it would be good if we wouldn't have to duplicate the code to serialize and send a message.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on pull request #1257:
dotnet: add session connection
Posted by "heljoyLiu (GitHub)" <gi...@apache.org>.
thanks, I add "session().close()" as tips for debug things, the processor will ignore it. I will add comment for this.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1257:
dotnet: add session connection
Posted by "FlorianHockmann (GitHub)" <gi...@apache.org>.
(nitpick) Again: **s**tring
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1257:
dotnet: add session connection
Posted by "FlorianHockmann (GitHub)" <gi...@apache.org>.
(nitpick) This shouldn't be necessary as you can also assign `message` again below:
```c#
message = RebuildSessionMessage(message);
```
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1257:
dotnet: add session connection
Posted by "FlorianHockmann (GitHub)" <gi...@apache.org>.
One other idea I just got, how about adding a private property like this:
```c#
private bool IsSessionEnabled => !string.IsNullOrEmpty(_sessionId);
```
then this if clause could be changed to:
```c#
if (IsSessionEnabled)
```
which is easer to understand in my opinion.
But this if course only a suggestion. Ignore it if you disagree.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on pull request #1257:
dotnet: add session connection
Posted by "heljoyLiu (GitHub)" <gi...@apache.org>.
I checked sessionId in document, here is `Guid`, but String request [in code](https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java#L210), and it fails when `Guid` in args
```
[WARN] OpExecutorHandler - java.util.UUID cannot be cast to java.lang.String
java.lang.ClassCastException: java.util.UUID cannot be cast to java.lang.String
at org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor.getSession(SessionOpProcessor.java:203)
```
anyway, we should update document or add `UUID` support in `gremlin-server`
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on issue #1257: dotnet: add
session connection
Posted by "heljoyLiu (GitHub)" <gi...@apache.org>.
> Thanks for the contribution, @heljoyLiu! The PR looks good at a first glance, but I'll need some more time to do a full review.
> I'm not sure if you're aware of that but the Gremlin.NET driver has changed a lot between `3.3-dev` and `3.4-dev`, so if you want this change to go into `3.3-dev` already, then it would be good if you could create another PR that targets `3.4-dev` with this contribution. That would make it a lot easier to get your contribution into all release branches.
ok, I will create another PR to `3.4-dev` after review, thanks @FlorianHockmann
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on issue #1257: dotnet: add
session connection
Posted by "heljoyLiu (GitHub)" <gi...@apache.org>.
@FlorianHockmann , I will prepare another PR for `3.4-dev` , pls let me know if any wrong, thanks.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on pull request #1257:
dotnet: add session connection
Posted by "heljoyLiu (GitHub)" <gi...@apache.org>.
thanks, got it
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on pull request #1257:
dotnet: add session connection
Posted by "heljoyLiu (GitHub)" <gi...@apache.org>.
I checked sessionId in document, here is `Guid`, but String request [in code](https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java#L210), and it fails when `Guid` in args
```
[WARN] OpExecutorHandler - java.util.UUID cannot be cast to java.lang.String
java.lang.ClassCastException: java.util.UUID cannot be cast to java.lang.String
at org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor.getSession(SessionOpProcessor.java:203)
```
anything, we should update document or add `UUID` support in `gremlin-server`
[ Full content available at: https://github.com/apache/tinkerpop/pull/1257 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org