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/12/08 18:05:04 UTC
[GitHub] [tinkerpop] lyndonb-bq opened a new pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
lyndonb-bq opened a new pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515
# 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](https://issues.apache.org/jira/browse/TINKERPOP-2555)
[TINKERPOP-? - Support for logger in gremlin-python](?)
[TINKERPOP-? - 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`
6. Added `Bytecode` support to `Session` `processor`
7. Fixed bug in `aiohttp` transport layer that popped up when it was not shutdown properly
8. Added `Transaction` class to `graph_traversal.py`, added `tx` function which generates a `Transaction`
9. Added `create_graph_op `method that allows a simple `Bytecode` with source to be created
10. 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
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765774083
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -129,24 +123,28 @@ def _get_connection(self):
headers=self._headers)
def submit(self, message, bindings=None, request_options=None):
- return self.submitAsync(message, bindings=bindings, request_options=request_options).result()
+ return self.submit_async(message, bindings=bindings, request_options=request_options).result()
- def submitAsync(self, message, bindings=None, request_options=None):
Review comment:
I think that renaming this is good but it is a breaking change for users. let's deprecate `submitAsync` for `submit_async` and have both with the old function just calling the new one. When merged to master, i will delete the `submitAsync` one.
note that we do use `warnings` (at least in one place) for deprecation:
```python
warnings.warn(
"As of release 3.3.5, replaced by the gremlin_python.process.anonymous_traversal.traversal() function.",
DeprecationWarning)
```
I suppose a code comment would suffice as well if you think the warning thing is too heavy
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765777877
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/aiohttp/transport.py
##########
@@ -16,12 +16,11 @@
# specific language governing permissions and limitations
# under the License.
#
-
-
import aiohttp
import asyncio
import async_timeout
from aiohttp import ClientResponseError
+import logging
Review comment:
It's cool that we have logging now. This is going to be a huge help. Elsewhere I'd commented that we use:
https://docs.python.org/3/library/warnings.html
in one place: `graph.py`. I believe there is a way to re-route warnings to the logger:
https://docs.python.org/3/library/logging.html#logging.captureWarnings
perhaps we should do that if you haven't already somewhere? or perhaps we don't need `warnings` anymore at all? or perhaps we should double down on usage? I'll leave it to you to make a choice on how to rectify `warnings` and `logging` but i think it would be good to have a strategy here.
--
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] spmallette commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-995339298
@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`?
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r766241105
##########
File path: gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py
##########
@@ -93,12 +98,30 @@ def with_(self, k, v=None):
def withRemote(self, remote_connection):
source = self.get_graph_traversal_source()
source.traversal_strategies.add_strategies([RemoteStrategy(remote_connection)])
+ self.remote_connection = remote_connection
return source
+ def tx(self):
+ # In order to keep the constructor unchanged within 3.5.x we can try to pop the RemoteConnection out of the
+ # TraversalStrategies. keeping this unchanged will allow user DSLs to not take a break.
+ # This is the same strategy as gremlin-javascript.
+ # TODO: refactor this to be nicer in 3.6.0 when we can take a breaking change
Review comment:
JIRA is fine. i sort of recall the JS approach and that was sorta forced in there a bit so i guess we should make a ticket for that too.
--
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 a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonb-bq commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r766213513
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py
##########
@@ -48,21 +67,25 @@ def __init__(self, url, traversal_source = "g", protocol_factory=None,
password=password,
kerberized_service=kerberized_service,
headers=headers,
+ session=session,
**transport_kwargs)
self._url = self._client._url
self._traversal_source = self._client._traversal_source
def close(self):
+ logging.info("closing DriverRemoteConnection with url '%s'", str(self._url))
self._client.close()
def submit(self, bytecode):
+ logging.debug("submit with bytecode '%s'", str(bytecode))
result_set = self._client.submit(bytecode, request_options=self._extract_request_options(bytecode))
results = result_set.all().result()
return RemoteTraversal(iter(results))
- def submitAsync(self, bytecode):
+ def submit_async(self, bytecode):
Review comment:
Sounds good, will do.
--
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] spmallette commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-993664983
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765788782
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py
##########
@@ -48,21 +67,25 @@ def __init__(self, url, traversal_source = "g", protocol_factory=None,
password=password,
kerberized_service=kerberized_service,
headers=headers,
+ session=session,
**transport_kwargs)
self._url = self._client._url
self._traversal_source = self._client._traversal_source
def close(self):
+ logging.info("closing DriverRemoteConnection with url '%s'", str(self._url))
self._client.close()
def submit(self, bytecode):
+ logging.debug("submit with bytecode '%s'", str(bytecode))
result_set = self._client.submit(bytecode, request_options=self._extract_request_options(bytecode))
results = result_set.all().result()
return RemoteTraversal(iter(results))
- def submitAsync(self, bytecode):
+ def submit_async(self, bytecode):
Review comment:
i asked you to keep the old `submitAsync` form in `Client` as it was a breaking change. not sure that it matter so much here because I don't think anyone calls this directly. maybe just to be safe, retain the old form in addition to the new and we'll remove the old on merge to `master`.
--
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] krlawrence commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
krlawrence commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-991331278
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
--
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 #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonb-bq commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-995417228
> @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.
--
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] spmallette merged pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765792310
##########
File path: gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py
##########
@@ -93,12 +98,30 @@ def with_(self, k, v=None):
def withRemote(self, remote_connection):
source = self.get_graph_traversal_source()
source.traversal_strategies.add_strategies([RemoteStrategy(remote_connection)])
+ self.remote_connection = remote_connection
return source
+ def tx(self):
+ # In order to keep the constructor unchanged within 3.5.x we can try to pop the RemoteConnection out of the
+ # TraversalStrategies. keeping this unchanged will allow user DSLs to not take a break.
+ # This is the same strategy as gremlin-javascript.
+ # TODO: refactor this to be nicer in 3.6.0 when we can take a breaking change
Review comment:
Are you expecting a simple refactoring here? Or perhaps more the point, once we merge this, will it be easy for you to submit a follow-on PR to `master` to do this or will we need to create JIRA for future consideration?
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765763491
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -107,20 +109,12 @@ def _fill_pool(self):
self._pool.put_nowait(conn)
def close(self):
- if self._sessionEnabled:
- self._close_session()
+ logging.info("Closing Client with url '%s'", self._url)
while not self._pool.empty():
conn = self._pool.get(True)
conn.close()
self._executor.shutdown()
- def _close_session(self):
Review comment:
You removed this little function and I didn't see it's behavior anywhere else. That effectively undoes the work of #1510 - could you please make sure the "close" message gets sent when a session is closed just prior to closing the websocket connection?
--
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 a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonb-bq commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r766212934
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -129,24 +123,28 @@ def _get_connection(self):
headers=self._headers)
def submit(self, message, bindings=None, request_options=None):
- return self.submitAsync(message, bindings=bindings, request_options=request_options).result()
+ return self.submit_async(message, bindings=bindings, request_options=request_options).result()
- def submitAsync(self, message, bindings=None, request_options=None):
+ def submit_async(self, message, bindings=None, request_options=None):
+ logging.debug("message '%s'", str(message))
+ args = {'gremlin': message, 'aliases': {'g': self._traversal_source}}
+ processor = ''
+ op = 'eval'
if isinstance(message, traversal.Bytecode):
- message = request.RequestMessage(
- processor='traversal', op='bytecode',
- args={'gremlin': message,
- 'aliases': {'g': self._traversal_source}})
- elif isinstance(message, str):
- message = request.RequestMessage(
- processor='', op='eval',
- args={'gremlin': message,
- 'aliases': {'g': self._traversal_source}})
- if bindings:
- message.args.update({'bindings': bindings})
- if self._sessionEnabled:
- message = message._replace(processor='session')
- message.args.update({'session': self._session})
+ op = 'bytecode'
+ processor = 'traversal'
+
+ if isinstance(message, str) and bindings:
+ args['bindings'] = bindings
+
+ if self._session_enabled:
+ args['session'] = str(self._session)
+ processor = 'session'
+
+ if isinstance(message, traversal.Bytecode) or isinstance(message, str):
+ logging.info("processor='%s', op='%s', args='%s'", str(processor), str(op), str(args))
Review comment:
I think you are right, I will switch to `debug`. I wish logging for python had an `all` equivalent.
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765787278
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -129,24 +123,28 @@ def _get_connection(self):
headers=self._headers)
def submit(self, message, bindings=None, request_options=None):
- return self.submitAsync(message, bindings=bindings, request_options=request_options).result()
+ return self.submit_async(message, bindings=bindings, request_options=request_options).result()
- def submitAsync(self, message, bindings=None, request_options=None):
+ def submit_async(self, message, bindings=None, request_options=None):
+ logging.debug("message '%s'", str(message))
+ args = {'gremlin': message, 'aliases': {'g': self._traversal_source}}
+ processor = ''
+ op = 'eval'
if isinstance(message, traversal.Bytecode):
- message = request.RequestMessage(
- processor='traversal', op='bytecode',
- args={'gremlin': message,
- 'aliases': {'g': self._traversal_source}})
- elif isinstance(message, str):
- message = request.RequestMessage(
- processor='', op='eval',
- args={'gremlin': message,
- 'aliases': {'g': self._traversal_source}})
- if bindings:
- message.args.update({'bindings': bindings})
- if self._sessionEnabled:
- message = message._replace(processor='session')
- message.args.update({'session': self._session})
+ op = 'bytecode'
+ processor = 'traversal'
+
+ if isinstance(message, str) and bindings:
+ args['bindings'] = bindings
+
+ if self._session_enabled:
+ args['session'] = str(self._session)
+ processor = 'session'
+
+ if isinstance(message, traversal.Bytecode) or isinstance(message, str):
+ logging.info("processor='%s', op='%s', args='%s'", str(processor), str(op), str(args))
Review comment:
i didnt give much thought yet to the logging messages. perhaps i'll do that on the next pass of this PR, but should we think about each of these log messages and whether or not they will make too much noise? e.g. "info" for this one seems like it would be a bit loud maybe at an `info` level?
--
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 edited a comment on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonb-bq edited a comment on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-995417228
> @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.
--
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 a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonb-bq commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r766215120
##########
File path: gremlin-python/src/main/python/gremlin_python/process/graph_traversal.py
##########
@@ -93,12 +98,30 @@ def with_(self, k, v=None):
def withRemote(self, remote_connection):
source = self.get_graph_traversal_source()
source.traversal_strategies.add_strategies([RemoteStrategy(remote_connection)])
+ self.remote_connection = remote_connection
return source
+ def tx(self):
+ # In order to keep the constructor unchanged within 3.5.x we can try to pop the RemoteConnection out of the
+ # TraversalStrategies. keeping this unchanged will allow user DSLs to not take a break.
+ # This is the same strategy as gremlin-javascript.
+ # TODO: refactor this to be nicer in 3.6.0 when we can take a breaking change
Review comment:
I used this technique because JavaScript did and I could not come up with a better non-breaking change here.
It would require a bit of remodelling and it's the kind of thing that could get forgotten, so I think a Jira ticket would be more appropriate.
If you agree I will make one and reference it here.
--
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] spmallette commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-995840322
nice - merged now - thank you!
--
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] spmallette commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-989830240
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: dd859b82987790d7e6ab57dc328c2c74135dd24a 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.
--
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 a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonb-bq commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r766212696
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -129,24 +123,28 @@ def _get_connection(self):
headers=self._headers)
def submit(self, message, bindings=None, request_options=None):
- return self.submitAsync(message, bindings=bindings, request_options=request_options).result()
+ return self.submit_async(message, bindings=bindings, request_options=request_options).result()
- def submitAsync(self, message, bindings=None, request_options=None):
Review comment:
That is a good point, I will do the warning / DeprecationWarning thing noted above.
--
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] krlawrence commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
krlawrence commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-991331278
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
--
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] spmallette commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765780921
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -31,16 +32,17 @@
def cpu_count():
return None
-__author__ = 'David M. Brown (davebshow@gmail.com)'
+__author__ = 'David M. Brown (davebshow@gmail.com), Lyndon Bauto (lyndonb@bitquilltech.com)'
class Client:
def __init__(self, url, traversal_source, protocol_factory=None,
transport_factory=None, pool_size=None, max_workers=None,
message_serializer=None, username="", password="",
- kerberized_service="", headers=None, session="",
+ kerberized_service="", headers=None, session=None,
Review comment:
smart
--
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] spmallette commented on pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#issuecomment-989865647
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!
--
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] krlawrence commented on a change in pull request #1515: TINKERPOP-2555 - Remote Transaction Support (g.tx()) for gremlin-python
Posted by GitBox <gi...@apache.org>.
krlawrence commented on a change in pull request #1515:
URL: https://github.com/apache/tinkerpop/pull/1515#discussion_r765871750
##########
File path: gremlin-python/src/main/python/gremlin_python/driver/client.py
##########
@@ -129,24 +123,28 @@ def _get_connection(self):
headers=self._headers)
def submit(self, message, bindings=None, request_options=None):
- return self.submitAsync(message, bindings=bindings, request_options=request_options).result()
+ return self.submit_async(message, bindings=bindings, request_options=request_options).result()
- def submitAsync(self, message, bindings=None, request_options=None):
Review comment:
I like the idea of warnings, especially when we will be changing code that has been this way for ages and is in production all over the place. I would prefer we keep both for a while rather than remove the old one.
--
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