Skip to content

Conversation

@lyndonbauto
Copy link
Contributor

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

Motivation

The current Tornado implementation of gremlin-python has a number of issues including heartbeat timeout, multithreading issues with DriverRemoteConnection, errors in the build due to the IOLoop, and issues with being unable to use the asyncio event loop when using the driver.

Because Python 2 support is dropped for Tinkerpop 3.5, this is a good time to switch to another implementation and do a clean cut of all these issues. This implementation uses AIOHTTP, this will also put gremlin-python one step closer to having a fully asyncio compatible solution that properly leverages the async/await syntax.

See more on the discussion here https://lists.apache.org/thread.html/rc4d047ba0245fde3261bad0ea156a0f35db1ab0f92c8e80d4fb1b815%40%3Cdev.tinkerpop.apache.org%3E

Changes

The following changes were made:

  • Removed Tornado
  • Added AIOHTTP implementation
  • Added dependency on AIOHTTP and removed dependency on Tornado
  • Added max_content_length and heartbeat parameters to transport
  • Removed compression_options parameter from transport
  • Added some unit tests
  • Switched transport.closed to abc.abstractmethod/@property from @abstractproperty since the latter is deprecated

Applicable Tickets

Ticket for change to be made https://issues.apache.org/jira/browse/TINKERPOP-2546

The following issues are fixed with these changes:

Testing Consideration

The following tests were done

  • Builds with mvn clean install -pl gremlin-python
  • Manually tested that if idleConnectionTimeout is set and a heartbeat is provided, connection does not drop
  • Manually tested that if idleConnectionTimeout is set and a heartbeat is NOT provided, connection does drop
  • Observed that the build output no longer prints IOLoop errors

…don/gremlin-python-asyncio-transport

[1] Minor updates to function definition style. Changed to inner functions from separate definitions
[2] Added handlers for different msg types
[3] Added more parameters
[2] Added and plumbed in max_content_length
[2] Added changelog
@spmallette
Copy link
Contributor

There are a few places in the documentation that make mention of tornado - they all seem to be in the "Python" section here:

https://github.com/apache/tinkerpop/blob/master/docs/src/reference/gremlin-variants.asciidoc#gremlin-python

If you could find those and make adjustments as necessary that would be great.

Also, this sort of change is the kind of thing we like to callout in our Upgrade Documentation, where we let folks know about major sorts of changes in new versions. If you'd like to draft something there in this PR that would be nice otherwise I can just take care of it after merge:

https://github.com/apache/tinkerpop/blob/master/docs/src/upgrade/release-3.5.x.asciidoc

[2] Added info in the upgrade about AIOHTTP change
@lyndonbauto
Copy link
Contributor Author

There are a few places in the documentation that make mention of tornado - they all seem to be in the "Python" section here:

https://github.com/apache/tinkerpop/blob/master/docs/src/reference/gremlin-variants.asciidoc#gremlin-python

If you could find those and make adjustments as necessary that would be great.

Also, this sort of change is the kind of thing we like to callout in our Upgrade Documentation, where we let folks know about major sorts of changes in new versions. If you'd like to draft something there in this PR that would be nice otherwise I can just take care of it after merge:

https://github.com/apache/tinkerpop/blob/master/docs/src/upgrade/release-3.5.x.asciidoc

Thanks for the feedback @spmallette, sorry I missed those references on the first pass. I have corrected them. Also I added the upgrade documentation, if you want me to add any more or less on it, please let me know!

See: link:https://issues.apache.org/jira/browse/TINKERPOP-2317[TINKERPOP-2317]
==== Python Transportation Layer Rewrite
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good - two things:

  1. please include the "See: " to link to the JIRA
  2. i think we should clearly state that Tornado support has been removed.

[2] Made test for functions called in the event loop
[2] Plumbed call_from_event_loop option through client and driver_remote_connection
@lyndonbauto
Copy link
Contributor Author

Closing this because I made a new pull request with cleaned up git history:
#1416

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