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.

Sign up for GitHub

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

Jump to bottom

TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python #1515

Merged
merged 5 commits into from
Dec 16, 2021
Merged

TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python #1515

merged 5 commits into from
Dec 16, 2021

Conversation

lyndonbauto
Copy link
Contributor

@lyndonbauto lyndonbauto commented Dec 8, 2021 •
edited
Loading

Summary

The following gremlin-python features and bugs are addressed in this pull request:

  • Remote Transaction support (g.tx())
  • Logging framework
  • Bug where improper cleanup causes aiohttp stack trace pop up
  • Fixed a bit of general code formatting throughout the library

Related tickets

TINKERPOP-2555 - g.tx() support for gremlin-python
TINKERPOP-2637 - Support for logger in gremlin-python
TINKERPOP-2662 - Improper closing of gremlin-python driver causes exceptions

High level overview of individual changes made

Source:

  1. Added neo4j-gremlin dependency to pom.xml, fixed issue with TEST_TRANSACTION environment variable.
  2. Made DriverRemoteConnection latch parameters so they can be reused to create a subsequent session
  3. Added logging throughout the driver
  4. Added commit and rollback to DriverRemoteConnection
  5. Added some logging to receive message
  6. Added transaction support to RemoteConnection
  7. Added Bytecode support to Session processor
  8. Fixed bug in aiohttp transport layer that popped up when it was not shutdown properly
  9. Added Transaction class to graph_traversal.py, added tx function which generates a Transaction
  10. Added create_graph_op method that allows a simple Bytecode with source to be created
  11. Added GraphOp class with commit and rollback functions which return Bytecode for commit and rollback

Test:

  1. Enhance test configuration to have remote_transaction_connection
  2. Updated submitAsync -> submit_async for test_client
  3. Added transaction test to exercise basic commit
  4. Added transaction test to exercise basic rollback
  5. Added transaction test to exercise multi commit
  6. Added transaction test to exercise multi rollback
  7. Added transaction test to exercise multi commit and rollback together
  8. Added transaction test to exercise illegal operations (negative test)

Documentation:

  1. Added documentation for gremlin-python usage
  2. Updated CHANGELOG to include g.tx() support, logging, and aiohttp shutdown fix in gremlin-python

@lyndonbauto
Source:
6f45069 
[1] Added transaction profile to pom.xml
[2] Made DriverRemoteConnection latch parameters so they can be reused to create a subsequent session
[3] Added logging throughout the driver
[4] Added commit and rollback to DriverRemoteConnection
[5] Added some logging to receive message
[6] Added transaction support to RemoteConnection
[7] Added bytecode support to Session processor
[8] Fixed bug in aiohttp transport layer that popped up when it was not shutdown properly
[9] Added Transaction class to graph_traversal.py, added tx function which generates a Transaction
[10] Added create_graph_op method that allows a simple Bytecode with source to be created
[11] Added GraphOp class with commit and rollback functions which return Bytecode for commit and rollback

Test:
[12] Enhance test configuration to have remote_transaction_connection
[13] Updated submitAsync -> submit_async for test_client
[14] Added transaction test to exercise basic commit
[15] Added transaction test to exercise basic rollback
[16] Added transaction test to exercise multi commit
[17] Added transaction test to exercise multi rollback
[18] Added transaction test to exercise multi commit and rollback together
[19] Added transaction test to exercise illegal operations (negative test)

Documentation:
[20] Added updated documentation for gtx implementation
@lyndonbauto
[1] Fixed TEST_TRANSACTION environment variable
dd796be 
[2] Enabling transaction tests in GitHub actions
@lyndonbauto
[1] Added session support to string messages. This was unintentionall…
2594cf5 
…y removed.
@lyndonbauto lyndonbauto marked this pull request as ready for review December 8, 2021 20:09
@spmallette
Copy link
Contributor

I restarted the tests and they pass now. I can't imagine why UnifiedChannelizer tests were failing as a result of this change. I did notice that the logs for that job were ridiculously large which led me to this little fix: dd859b8 I don't know if that was somehow causing the failure as i could not recreate the problem. In any case, please rebase so as to include that little change so that any future runs that fail for UnifiedChannelizer at least give us some clean logs to look at.

@spmallette
Copy link
Contributor

I've made a first pass at this. I'll give it another when you address these initial comments, but overall it looks good and it seems like there is little to really change of substance. From a documentation perspective could you also please add a section like this to the Upgrade Docs:

https://github.com/apache/tinkerpop/blob/dd859b82987790d7e6ab57dc328c2c74135dd24a/docs/src/upgrade/release-3.5.x.asciidoc#upgrading-for-users

and then section in the Reference Documentation like this under the Python section:

https://github.com/apache/tinkerpop/blob/dd859b82987790d7e6ab57dc328c2c74135dd24a/docs/src/reference/gremlin-variants.asciidoc#transactions-2

You did a really nice job with this! This is a great contribution - Thanks!

@lyndonbauto
Added submitAsync in Client and DriverRemoteConnection with deprecate…
d1e3abd 
…d message

Fixed missing session close in Client
Switched info to debug log for heavy spam messages
Added gremlin-variant remote transaction documentation for gremlin-python
Added release documentation for remote transactions in gremlin-python
@krlawrence
Copy link
Contributor

I have not had a chance to build this branch but I have taken time to look over all of the changes for TX and AioHTTP closing issues and it looks good to me. Thanks for all the work on this one.

VOTE +1

@spmallette
Copy link
Contributor

VOTE +1

@spmallette
Copy link
Contributor

@lyndonb-bq - i tried to build this without -DincludeNeo4j and it's failing because neo4j isn't configured. Maybe it's not skipping those tests properly when transaction testing environment variable isn't enabled? Do you have this problem with a regular mvn clean install -pl gremlin-python?

@lyndonbauto
Copy link
Contributor Author

lyndonbauto commented Dec 16, 2021 •
edited
Loading

@lyndonb-bq - i tried to build this without -DincludeNeo4j and it's failing because neo4j isn't configured. Maybe it's not skipping those tests properly when transaction testing environment variable isn't enabled? Do you have this problem with a regular mvn clean install -pl gremlin-python?

@spmallette I missed that on my first pass, thanks for catching it. I have corrected this behaviour in my recent commit and verified it on my local machine. I tried both ways (with and without -DincludeNeo4j and both work on my local.

@spmallette spmallette merged commit fee9056 into apache:3.5-dev Dec 16, 2021
@spmallette
Copy link
Contributor

nice - merged now - thank you!

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

@krlawrence krlawrence Awaiting requested review from krlawrence

@spmallette spmallette Awaiting requested review from spmallette

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lyndonbauto @spmallette @krlawrence