[GitHub] [arrow] pitrou commented on a change in pull request #7406: ARROW-9093: [FlightRPC][C++][Python] expose generic gRPC transport options

[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