Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session support #116

Merged
merged 4 commits into from
Feb 27, 2021
Merged

Session support #116

merged 4 commits into from
Feb 27, 2021

Conversation

maldrake
Copy link
Contributor

This PR provides an initial implementation of session support. If merged, it should close #113.

  • The change adds two functions, openSession and closeSession to the GremlinClient, both synchronous and asynchronous.
  • Internally, the openSession function creates a new pool with just a single connection for the session, along with tracking the session name, which the client of the library will most often set to the String representation of a UUID. An alternative would've been to just create a single Connection rather than a pool of 1, but using the pool allowed for greater re-use of existing functions without changes, because many of the functions involved in sending messages take a managed connection from the pool.
  • Queries that are sent using execute are sent to the session processor wi the session name, until the session is closed.

Notably, I did learn a few things that might be viewed as limitations on the implementation.

  • Sessions are supported only for text queries, not binary traversals. This seems to be a design choice on the server end, so I don't think there's anything that can be done about it in the client.
  • The server sends WebSocket ping requests to the client as keep-alive test. The Gremlin client currently doesn't recognize or know how to handle those pings (by sending a pong response). Implementing robust handling for those would likely require either a monitoring thread to read and respond to them, or an asynchronous equivalent. For now, I resolved the problem by setting idleConnectionTimeout and keepAliveInterval to disable the ping/pong keep-alive messages altogether, to avoid that complexity. That should work fine for my use case but could be a limitation for others.

As always, I welcome any review comments or requests for changes that I need to make in order for the PR to be suitable to merge.

@wolf4ood
Copy link
Owner

wolf4ood commented Jan 3, 2021

Hi @maldrake

Thanks for the PR looks great!.

I was just thinking if it make sense to create another instance GremlinClient on open_session without modifying the current client or create a new type which wraps the GremlinClient with the session logic?
WDYT?

@maldrake
Copy link
Contributor Author

maldrake commented Jan 5, 2021

@wolf4ood,

Yes, that makes a lot of sense. I hadn't done that initially because I thought it would make it easier to interoperate with other parts of the code (e.g. traversals), if I could avoid changing the GremlinClient type. That was before I realized sessions don't support binary traversals, and I never went back to re-think that design later. I'll work up a change to the PR submit it soon.

@maldrake
Copy link
Contributor Author

maldrake commented Jan 5, 2021

Ok, in this new commit, I changed create_session to create a new client under a newtype of SessionedClient. How does this look?

@maldrake
Copy link
Contributor Author

Hi @wolf4ood, I wanted to check in on this PR, in case it fell to the wayside in favor of other priorities. Any other changes I can make, to make it more suitable for merging? Thanks!

@wolf4ood wolf4ood merged commit 3c388a4 into wolf4ood:master Feb 27, 2021
@wolf4ood
Copy link
Owner

Hi @maldrake

sorry i did completely miss the notification on this.
Thansk for this PR :)

@maldrake maldrake deleted the session-support branch May 31, 2021 14:12
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.

Adding Session Support
2 participants