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 2021/04/13 19:35:57 UTC

[GitHub] [tinkerpop] lyndonb-bq opened a new pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

lyndonb-bq opened a new pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416


   Switching branch to fix commit history, old pull request is here: https://github.com/apache/tinkerpop/pull/1415
   
   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:
   - Heartbeat timeout issue: https://issues.apache.org/jira/browse/TINKERPOP-1886
   - DriverRemoteConnection multithreading issue: https://issues.apache.org/jira/browse/TINKERPOP-2388
   - IOLoop close build errors: https://issues.apache.org/jira/browse/TINKERPOP-2484
   
   ### 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


-- 
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

[GitHub] [tinkerpop] rdpravin1895 commented on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

rdpravin1895 commented on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-948362199


   @lyndonb-bq Is there a documentation to know how the heartbeat parameters can be used? 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tinkerpop] rdpravin1895 removed a comment on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

rdpravin1895 removed a comment on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-948362199


   @lyndonb-bq Is there a documentation to know how the heartbeat parameters can be used? 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tinkerpop] lyndonb-bq commented on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

lyndonb-bq commented on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-819726446


   > i recall you saying that the compression dict map was removed as there was not quite an analogous configuration for aiohttp but i though that there was a compression option in aiohttp that could be enabled as a flag still but i didn't see that here. am i missing where that is somehow?
   
   @spmallette There is a compression you can set for it but from what I have read it is a different type than Tornado's. I tried setting it up and all the tests failed whenever I used it so I don't think it is compatible with the current Gremlin server. I looked at aiogremlin's driver and found they didn't have it setup there either so I thought it might just not work for this use case unfortunately. 


-- 
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

[GitHub] [tinkerpop] spmallette commented on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

spmallette commented on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-822794094


   VOTE +1


-- 
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

[GitHub] [tinkerpop] spmallette commented on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

spmallette commented on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-819423801


   i recall you saying that the compression dict map was removed as there was not quite an analogous configuration for aiohttp but i though that there was a compression option in aiohttp that could be enabled as a flag still but i didn't see that here. am i missing where that is somehow?


-- 
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

[GitHub] [tinkerpop] lyndonb-bq commented on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

lyndonb-bq commented on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-819794303


   I updated the changes to allow keyword arg passthrough directly. This way a user can just directly pass through anything available in https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientSession.ws_connect
   I also mapped ssl_options to ssl and max_content_length to max_msg_size for backwards compatibility if users already have those being set.


-- 
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

[GitHub] [tinkerpop] spmallette merged pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

spmallette merged pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416


   


-- 
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

[GitHub] [tinkerpop] krlawrence commented on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

krlawrence commented on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-821513582


   I have run a lot of tests using this code as a replacement for the Tornado based version and with the last few additional commits have found no additional issues. We have had a lot of really tricky event loop conflict issues with Tornado and systems like Jupyter. These changes should alleviate that situation significantly.
   
   VOTE +1


-- 
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

[GitHub] [tinkerpop] spmallette edited a comment on pull request #1416: TINKERPOP-2546 Gremlin-Python Client Transport Layer to use AIOHTTP

Posted by GitBox <gi...@apache.org>.

spmallette edited a comment on pull request #1416:
URL: https://github.com/apache/tinkerpop/pull/1416#issuecomment-822794094


   VOTE +1 - i plan to merge this tomorrow at some point. it's been open for a while with lots of discussion, so i would think that if someone had concerns they would have been raised already. 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