kenhuuu commented on code in PR #3469: URL: https://github.com/apache/tinkerpop/pull/3469#discussion_r3444683368
########## gremlin-python/src/main/python/gremlin_python/driver/client.py: ########## @@ -38,21 +38,34 @@ def cpu_count(): class Client: - def __init__(self, url, traversal_source, pool_size=None, max_workers=None, + def __init__(self, url, traversal_source, max_connections=None, max_workers=None, Review Comment: This is bit of a legacy question, but any reason why we don't just set the defaults here rather than checking for None later and setting them after? ########## gremlin-python/src/main/python/tests/unit/driver/test_transport_compression.py: ########## @@ -0,0 +1,178 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +"""Loopback round-trip tests for the aiohttp transport's compression handling. + +These spin up a tiny in-process aiohttp server on an ephemeral port (no +external network) to verify the *actual* over-the-wire behavior that the +unit-level mocks cannot: + + - M1: the outgoing Accept-Encoding header the server receives for the + 'none' vs 'deflate' compression options. + - M2: a real deflate-compressed response is transparently decompressed when + read back through the streaming get_stream() path. +""" + +import asyncio +import threading +import time +import zlib + +import pytest + +from gremlin_python.driver.aiohttp.transport import AiohttpHTTPTransport + + +class _LoopbackServer: + """Runs an aiohttp app on its own event loop in a background thread. + + The handler records the Accept-Encoding header it received and replies with + a deflate-compressed body (advertising Content-Encoding: deflate) so the + client must decompress to recover the original payload. + """ + + def __init__(self, payload): + self._payload = payload + self.received_accept_encoding = "<unset>" + self._loop = None + self._runner = None + self._thread = None + self.port = None + self._started = threading.Event() + + async def _handler(self, request): + from aiohttp import web + self.received_accept_encoding = request.headers.get('Accept-Encoding', None) + body = zlib.compress(self._payload) + return web.Response(body=body, + headers={'Content-Encoding': 'deflate', + 'Content-Type': 'application/octet-stream'}) + + def _run(self): + from aiohttp import web + self._loop = asyncio.new_event_loop() + asyncio.set_event_loop(self._loop) + + async def _setup(): + app = web.Application() + app.router.add_post('/', self._handler) + self._runner = web.AppRunner(app) + await self._runner.setup() + site = web.TCPSite(self._runner, '127.0.0.1', 0) + await site.start() + self.port = site._server.sockets[0].getsockname()[1] + self._started.set() + + self._loop.run_until_complete(_setup()) + self._loop.run_forever() + + def __enter__(self): + self._thread = threading.Thread(target=self._run, daemon=True) + self._thread.start() + assert self._started.wait(timeout=10), "loopback server failed to start" + return self + + def __exit__(self, *exc): + async def _cleanup(): + await self._runner.cleanup() + fut = asyncio.run_coroutine_threadsafe(_cleanup(), self._loop) + try: + fut.result(timeout=10) + finally: + self._loop.call_soon_threadsafe(self._loop.stop) + self._thread.join(timeout=10) + + @property + def url(self): + return 'http://127.0.0.1:%d/' % self.port + + +def _read_all(stream): + """Drain an AiohttpSyncStream, tolerating the IncompleteReadError raised at + EOF (the deserializer normally stops on the end-of-stream marker before + hitting EOF, but here we read raw bytes to completion).""" + out = bytearray() + while True: + try: + chunk = stream.read(4096) + except asyncio.IncompleteReadError as e: + out.extend(e.partial) + break + if not chunk: + break + out.extend(chunk) + return bytes(out) + + +class TestCompressionWireBehavior: Review Comment: I'm not sure what value these tests bring. The GremlinServer already supports deflate so if we just enabled compression during integration tests, it already verifies that the driver can properly handle deflate compression. The only thing that needs to be verified is that the request header requests for compression. ########## docs/src/reference/gremlin-variants.asciidoc: ########## @@ -3188,13 +3190,21 @@ can be passed to the `Client` or `DriverRemoteConnection` instance as keyword ar [width="100%",cols="3,10,^2",options="header"] |========================================================= |Key |Description |Default -|headers |Additional headers that will be added to each request message. |`None` -|max_workers |Maximum number of worker threads. |Number of CPUs * 5 -|request_serializer |The request serializer implementation.|`gremlin_python.driver.serializer.GraphBinarySerializersV4` +|max_workers |Maximum number of worker threads. |Same as `max_connections` (128) |response_serializer |The response serializer implementation.|`gremlin_python.driver.serializer.GraphBinarySerializersV4` -|interceptors |The request interceptors to run after request serialization.|`None` +|interceptors |The request interceptors to run before the request body is serialized.|`None` |auth |An authentication interceptor. Always appended to the end of the interceptor list so it runs last. |`None` -|pool_size |The number of connections used by the pool. |4 +|max_connections |The maximum number of connections used by the pool. `pool_size` is accepted as a deprecated alias. |128 +|connect_timeout |Timeout in seconds for establishing the connection (TCP connect plus TLS handshake). |5 +|read_timeout |Per-read idle timeout in seconds applied while streaming a response. Resets per chunk. |`None` +|write_timeout |Timeout in seconds for writing a request to the transport. |`None` +|ssl |An `ssl.SSLContext` used for TLS connections. `ssl_options` is accepted as a deprecated alias. |`None` +|idle_timeout |How long in seconds an idle connection remains in the pool before being closed. |180 Review Comment: This works differently from how it does in Java. May need to make this more consistent. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
