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/02 18:27:32 UTC
[GitHub] [tinkerpop] spmallette opened a new pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
spmallette opened a new pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413
https://issues.apache.org/jira/browse/TINKERPOP-2457
I'm not sure how this will merge to `master` at the moment, or whether that merge is relevant there given the work to move from tornado to aiohttp. I sense this work will merge before that, so we'll see how it becomes relevant or not.
Builds with `mvn clean install -pl gremlin-python`
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] krlawrence commented on pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
krlawrence commented on pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413#issuecomment-812656179
This is a commonly asked for feature that I have had to use private workarounds for in the past. Great to see this going into the mainline code.
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 a change in pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413#discussion_r609050796
##########
File path: gremlin-python/src/main/jython/gremlin_python/driver/tornado/transport.py
##########
@@ -26,21 +26,25 @@
class TornadoTransport(AbstractBaseTransport):
+ _default_max_content_length = 10 * 1024 * 1024
Review comment:
it was set to precisely the same default as existed before. isn't that correct @krlawrence ?
--
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 a change in pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
krlawrence commented on a change in pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413#discussion_r609068012
##########
File path: gremlin-python/src/main/jython/gremlin_python/driver/tornado/transport.py
##########
@@ -26,21 +26,25 @@
class TornadoTransport(AbstractBaseTransport):
+ _default_max_content_length = 10 * 1024 * 1024
Review comment:
The Tornado default is 10MB The Tornado code includes that exact calculation (10 * 1024 * 1024) in fact. This should not be a breaking change to anyone. I did a lot of testing of this before I opened the original Jira to prove that that is the current limit.
--
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] divijvaidya commented on pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413#issuecomment-815839263
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 merged pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413
--
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] divijvaidya commented on a change in pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413#discussion_r609720031
##########
File path: gremlin-python/src/main/jython/gremlin_python/driver/tornado/transport.py
##########
@@ -26,21 +26,25 @@
class TornadoTransport(AbstractBaseTransport):
+ _default_max_content_length = 10 * 1024 * 1024
Review comment:
Thank you for clarifying. If the default is same as before then this is definitely not breaking. That answers my question.
--
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] divijvaidya commented on a change in pull request #1413: TINKERPOP-2457 Added max_content_length as Python driver config
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1413:
URL: https://github.com/apache/tinkerpop/pull/1413#discussion_r609036552
##########
File path: gremlin-python/src/main/jython/gremlin_python/driver/tornado/transport.py
##########
@@ -26,21 +26,25 @@
class TornadoTransport(AbstractBaseTransport):
+ _default_max_content_length = 10 * 1024 * 1024
Review comment:
This is probably a breaking change and should go either in 350 or in 3411 with same default as current version. In absence of explicitly setting this value, the default must be coming from the websocket framework.
This is breaking because let's say that the current default is 20MB and a user of the client is relying on that but when we introduce a new default value of 10MB, after upgrade, their queries will suddenly start failing.
--
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