[GitHub] [arrow-rs] lidavidm commented on a change in pull request #1402: fix: Fix grpc schema hack in flight integration test

[GitBox <gi...@apache.org>]

lidavidm commented on a change in pull request #1402:
URL: https://github.com/apache/arrow-rs/pull/1402#discussion_r820096763



##########
File path: integration-testing/src/flight_client_scenarios/integration_test.rs
##########
@@ -185,7 +185,8 @@ async fn consume_flight_location(
     let mut location = location;
     // The other Flight implementations use the `grpc+tcp` scheme, but the Rust http libs
     // don't recognize this as valid.
-    location.uri = location.uri.replace("grpc+tcp://", "grpc://");
+    // more details: https://github.com/apache/arrow-rs/issues/1398
+    location.uri = location.uri.replace("grpc+tcp://", "http://");

Review comment:
       Just some context on this bit: Flight's `Location` is never actually directly used as a gRPC URI. The C++ and Java implementations take apart and reconstruct the URI, so, grpc+tcp was never menat to be passed to gRPC. You can see here: https://github.com/apache/arrow/blob/4ef95eb89f9202dfcd9017633cf55671d56e337f/cpp/src/arrow/flight/client.cc#L935-L939
   
   I do wonder if gRPC got more strict. The last passing run used gRPC 1.43.2, the first failing run used gRPC 1.44.0. By my read, this is the responsible commit, first seen in gRPC 1.44.0: https://github.com/grpc/grpc/commit/0deb64d1f6461b96a65ed847e63251a54856654f
   
   It adds the error message to hpack_parser.cc (see ReportMetadataParseError) and crucially adds new validation to metadata_batch.h (see HttpSchemeMetadata). So I guess this was new validation added in v1.44.0.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org