You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/06/11 14:45:34 UTC
[GitHub] [arrow] lidavidm opened a new pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
lidavidm opened a new pull request #7406:
URL: https://github.com/apache/arrow/pull/7406
This allows passing generic client options to the underlying gRPC client in C++/Python.
The motivation is to expose these options: https://grpc.github.io/grpc/cpp/group__grpc__arg__keys.html which can be useful for tuning. For instance, we can then enable round-robin load balancing, useful in production deployments, or we can experiment with enabling SO_ZEROCOPY (which is now experimental in gRPC), without having to individually bind all of these options in Flight.
(Java doesn't require this due to the flight-grpc module, which gives you the underlying gRPC client/server resources; unfortunately a similar approach can't be done for Python since gRPC-Python is not based on gRPC-C++.)
----------------------------------------------------------------
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] [arrow] lidavidm commented on a change in pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#discussion_r438958815
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -1428,3 +1428,25 @@ def test_middleware_multi_header():
for header, values in MultiHeaderClientMiddleware.EXPECTED.items():
assert client_headers.get(header) == values
assert headers.last_headers.get(header) == values
+
+
+@pytest.mark.requires_testing_data
+def test_generic_options():
+ """Test setting generic client options."""
+ certs = example_tls_certs()
+
+ with ConstantFlightServer(tls_certificates=certs["certificates"]) as s:
Review comment:
We do - there are two tests here, using the same properties as C++. I can split them if that's clearer.
----------------------------------------------------------------
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] [arrow] lidavidm commented on pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#issuecomment-642890070
And rebased, thank you for all the reviews! :slightly_smiling_face:
----------------------------------------------------------------
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] [arrow] pitrou commented on pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#issuecomment-642890649
Well, thank you for improving and polishing Flight ;-)
----------------------------------------------------------------
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] [arrow] pitrou commented on pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#issuecomment-642857772
Ok, need to resolve conflicts now.
----------------------------------------------------------------
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] [arrow] github-actions[bot] commented on pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#issuecomment-642715859
https://issues.apache.org/jira/browse/ARROW-9093
----------------------------------------------------------------
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] [arrow] lidavidm commented on pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#issuecomment-642707518
In particular the zerocopy flag may be interesting to enable by default: https://github.com/grpc/grpc/commit/48f026d90ece794eb718d7749e0b54b83ef76feb
> For large RPCs (>= 16KiB) it reduces the amount of CPU time spent
since it avoids a userspace to kernel data buffer copy.
----------------------------------------------------------------
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] [arrow] pitrou commented on a change in pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#discussion_r438932983
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1003,6 +1009,14 @@ cdef class FlightClient:
<shared_ptr[CClientMiddlewareFactory]>
make_shared[CPyClientMiddlewareFactory](
<PyObject*> factory, start_call))
+ if generic_options:
+ for key, value in generic_options:
+ if isinstance(value, (str, bytes)):
+ variant = CIntStringVariant(<c_string> tobytes(value))
+ else:
+ variant = CIntStringVariant(<int> value)
+ c_options.generic_options.push_back(
Review comment:
Ah, too bad Cython doesn't support `emplace_back`.
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1601,6 +1620,26 @@ TEST_F(TestTls, OverrideHostname) {
ASSERT_RAISES(IOError, client->DoAction(options, action, &results));
}
+// Test the facility for setting generic transport options.
+TEST_F(TestTls, OverrideHostnameGeneric) {
+ std::unique_ptr<FlightClient> client;
+ auto client_options = FlightClientOptions();
+ client_options.generic_options.emplace_back(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG,
+ "fakehostname");
+ CertKeyPair root_cert;
+ ASSERT_OK(ExampleTlsCertificateRoot(&root_cert));
+ client_options.tls_root_certs = root_cert.pem_cert;
+ ASSERT_OK(FlightClient::Connect(location_, client_options, &client));
+
+ FlightCallOptions options;
+ options.timeout = TimeoutDuration{5.0};
+ Action action;
+ action.type = "test";
+ action.body = Buffer::FromString("");
+ std::unique_ptr<ResultStream> results;
+ ASSERT_RAISES(IOError, client->DoAction(options, action, &results));
Review comment:
Is the error message sufficiently stable to check something about it?
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -960,12 +960,17 @@ cdef class FlightClient:
Override the hostname checked by TLS. Insecure, use with caution.
middleware : list optional, default None
A list of ClientMiddlewareFactory instances.
+ generic_options : list optional, default None
+ A list of generic (string, int or string) option tuples passed
+ to the underlying transport. Effect is implementation
+ dependent.
"""
cdef:
unique_ptr[CFlightClient] client
def __init__(self, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None, middleware=None):
+ private_key=None, override_hostname=None, middleware=None,
+ generic_options=None):
Review comment:
Since we're augmenting this, I think we should make most arguments keyword-only, i.e.:
```python
def __init__(self, location, *, tls_root_certs=None, cert_chain=None,
private_key=None, override_hostname=None, middleware=None,
generic_options=None):
```
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -1428,3 +1428,25 @@ def test_middleware_multi_header():
for header, values in MultiHeaderClientMiddleware.EXPECTED.items():
assert client_headers.get(header) == values
assert headers.last_headers.get(header) == values
+
+
+@pytest.mark.requires_testing_data
+def test_generic_options():
+ """Test setting generic client options."""
+ certs = example_tls_certs()
+
+ with ConstantFlightServer(tls_certificates=certs["certificates"]) as s:
Review comment:
Wouldn't it be simpler to test with `GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH` as in C++?
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -2411,7 +2425,7 @@ cdef class FlightServerBase:
def connect(location, tls_root_certs=None, cert_chain=None, private_key=None,
Review comment:
Same here: make all arguments except `location` keyword-only?
----------------------------------------------------------------
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] [arrow] pitrou closed pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7406:
URL: https://github.com/apache/arrow/pull/7406
----------------------------------------------------------------
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] [arrow] pitrou commented on a change in pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#discussion_r438970393
##########
File path: python/pyarrow/tests/test_flight.py
##########
@@ -1428,3 +1428,25 @@ def test_middleware_multi_header():
for header, values in MultiHeaderClientMiddleware.EXPECTED.items():
assert client_headers.get(header) == values
assert headers.last_headers.get(header) == values
+
+
+@pytest.mark.requires_testing_data
+def test_generic_options():
+ """Test setting generic client options."""
+ certs = example_tls_certs()
+
+ with ConstantFlightServer(tls_certificates=certs["certificates"]) as s:
Review comment:
Ah, fine, I had misread.
----------------------------------------------------------------
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] [arrow] lidavidm commented on a change in pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #7406:
URL: https://github.com/apache/arrow/pull/7406#discussion_r438958440
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1601,6 +1620,26 @@ TEST_F(TestTls, OverrideHostname) {
ASSERT_RAISES(IOError, client->DoAction(options, action, &results));
}
+// Test the facility for setting generic transport options.
+TEST_F(TestTls, OverrideHostnameGeneric) {
+ std::unique_ptr<FlightClient> client;
+ auto client_options = FlightClientOptions();
+ client_options.generic_options.emplace_back(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG,
+ "fakehostname");
+ CertKeyPair root_cert;
+ ASSERT_OK(ExampleTlsCertificateRoot(&root_cert));
+ client_options.tls_root_certs = root_cert.pem_cert;
+ ASSERT_OK(FlightClient::Connect(location_, client_options, &client));
+
+ FlightCallOptions options;
+ options.timeout = TimeoutDuration{5.0};
+ Action action;
+ action.type = "test";
+ action.body = Buffer::FromString("");
+ std::unique_ptr<ResultStream> results;
+ ASSERT_RAISES(IOError, client->DoAction(options, action, &results));
Review comment:
I don't think so, unfortunately. I've added a comment to that effect.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -960,12 +960,17 @@ cdef class FlightClient:
Override the hostname checked by TLS. Insecure, use with caution.
middleware : list optional, default None
A list of ClientMiddlewareFactory instances.
+ generic_options : list optional, default None
+ A list of generic (string, int or string) option tuples passed
+ to the underlying transport. Effect is implementation
+ dependent.
"""
cdef:
unique_ptr[CFlightClient] client
def __init__(self, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None, middleware=None):
+ private_key=None, override_hostname=None, middleware=None,
+ generic_options=None):
Review comment:
Done (same below as well).
----------------------------------------------------------------
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