Skip to content

gremlin-python: add session mode client#1264

Closed
heljoyLiu wants to merge 2 commits intoapache:3.4-devfrom
heljoyLiu:3.4-dev-py-session
Closed

gremlin-python: add session mode client#1264
heljoyLiu wants to merge 2 commits intoapache:3.4-devfrom
heljoyLiu:3.4-dev-py-session

Conversation

@heljoyLiu
Copy link
Contributor

No description provided.

@heljoyLiu
Copy link
Contributor Author

hi @spmallette , it's for gremlin-python

@spmallette
Copy link
Contributor

I referenced this one from #1263 (3.4-dev)which added sessions for .NET. it is linked with #1257 (3.3-dev) and #1251 (javascript) With the addition of this PR it would mean that we have session support across all internally managed drivers. So far, I'm not sure if any of these changes have added any documentation and that needs to be rectified with the merge of this one.

Ideally we need:

  1. CHANGELOG entry.
  2. Upgrade documentation.
  3. Adjust text of "Considering Sessions" section in the Reference Documentation and add examples in each language.

I think for the first two items we can just refer to this feature generally - no need to have one changelog/upgrade section per language.

# close session, 'gremlin' in args as tips and not be executed actually
gremlin = args.get('gremlin')
if not gremlin:
args['gremlin'] = 'session.close()'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the other languages, I'd prefer we not include any extra arguments to the "close" message. Please modify this python PR to match what was done on the other language PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

en. I will remove this like PR in .Net

@spmallette
Copy link
Contributor

@heljoyLiu If you'd like to try to include the documentation changes I suggested above on this PR that would be great, otherwise I will look to add them myself on merge. Also note that you intended to fix this in Javascript:

https://github.com/apache/tinkerpop/pull/1251/files#diff-9ec1348d608a722cece8543e6db87a23R385

If you'd like to add that here on this PR that will be fine, though for some reason you target 3.4-dev with this PR but all other languages went to 3.3-dev - I think this change should be re-targetted to 3.3-dev to be consistent at this point. Please let me know if you think differently for some reason.

As a reminder to myself, on the merge of this one to master we probably need to review the impact of:

https://issues.apache.org/jira/browse/TINKERPOP-2336

https://github.com/apache/tinkerpop/blob/0a7769eea651ab0e72d3a6a0bd424fbb1197bd47/docs/src/upgrade/release-3.5.x.asciidoc#session-close

in the context of python/js/.net merging to master. Ultimately we want consistency of behavior across all the drivers, so these should be modified to match java (i.e. remove the send of the close message and validate that the close of the channel kills the session).

Well, I hope I've captured all the loose ends on this "session" feature. If anyone has anything else they might add please comment. Thanks.

@heljoyLiu
Copy link
Contributor Author

If you'd like to add that here on this PR that will be fine, though for some reason you target 3.4-dev with this PR but all other languages went to 3.3-dev - I think this change should be re-targetted to 3.3-dev to be consistent at this point. Please let me know if you think differently for some reason.

ok, I will re-target to 3.3-dev branch

@heljoyLiu
Copy link
Contributor Author

I think for the first two items we can just refer to this feature generally - no need to have one changelog/upgrade section per language.

I see, another PR for 3.3-dev is ready, and I will make one for JavaScript for close message too.
To the end, update the first two items

@heljoyLiu
Copy link
Contributor Author

thanks @spmallette, I will close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants