Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix grpc schema hack in flight integration test #1402

Merged
merged 1 commit into from
Mar 5, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Mar 5, 2022

Which issue does this PR close?

Closes #1398

Rationale for this change

Certain flight integration tests are failing with rust

We think something about the gRPC library has changed recently to be more strict, but I never found a good way to verify this.

What changes are included in this PR?

Change some workaround code in the "scheme" passed to gRPC to use http:// rather thangrpc:// #1398 (comment) as suggested by @lidavidm ❤️ ❤️

Are there any user-facing changes?

No this is a test only change

@alamb alamb added the development-process Related to development process of arrow-rs label Mar 5, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1402 (c8fb66d) into master (afcf304) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head c8fb66d differs from pull request most recent head 6132b3b. Consider uploading reports for the commit 6132b3b to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1402      +/-   ##
==========================================
- Coverage   83.10%   83.10%   -0.01%     
==========================================
  Files         181      181              
  Lines       53244    53244              
==========================================
- Hits        44249    44248       -1     
- Misses       8995     8996       +1     
Impacted Files Coverage Δ
...ng/src/flight_client_scenarios/integration_test.rs 0.00% <0.00%> (ø)
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (-0.46%) ⬇️
arrow/src/array/transform/mod.rs 86.43% <0.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afcf304...6132b3b. Read the comment docs.

@alamb alamb mentioned this pull request Mar 5, 2022
@alamb
Copy link
Contributor Author

alamb commented Mar 5, 2022

🎉
Screen Shot 2022-03-05 at 7 03 04 AM

Comment on lines 186 to +189
// 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://");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: grpc/grpc@0deb64d

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.

@alamb alamb merged commit ab41c25 into apache:master Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of arrow-rs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration Test is failing on master branch
3 participants