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 2022/03/05 11:14:07 UTC
[GitHub] [arrow-rs] alamb opened a new pull request #1402: fix: Fix grpc schema hack in flight integration test
alamb opened a new pull request #1402:
URL: https://github.com/apache/arrow-rs/pull/1402
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/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 than`grpc://` https://github.com/apache/arrow-rs/issues/1398#issuecomment-1059610103 as suggested by @lidavidm ❤️ ❤️
# Are there any user-facing changes?
No this is a test only change
--
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
[GitHub] [arrow-rs] alamb commented on pull request #1402: fix: Fix grpc schema hack in flight integration test
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1402:
URL: https://github.com/apache/arrow-rs/pull/1402#issuecomment-1059751083
🎉
--
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
[GitHub] [arrow-rs] lidavidm commented on a change in pull request #1402: fix: Fix grpc schema hack in flight integration test
Posted by 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
[GitHub] [arrow-rs] alamb edited a comment on pull request #1402: fix: Fix grpc schema hack in flight integration test
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #1402:
URL: https://github.com/apache/arrow-rs/pull/1402#issuecomment-1059751083
🎉
--
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
[GitHub] [arrow-rs] alamb merged pull request #1402: fix: Fix grpc schema hack in flight integration test
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1402:
URL: https://github.com/apache/arrow-rs/pull/1402
--
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
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1402: fix: Fix grpc schema hack in flight integration test
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1402:
URL: https://github.com/apache/arrow-rs/pull/1402#issuecomment-1059746563
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1402](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8fb66d) into [master](https://codecov.io/gh/apache/arrow-rs/commit/afcf3046514a474e9fd4b6bb93c078f1279a7c72?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (afcf304) will **decrease** coverage by `0.00%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head c8fb66d differs from pull request most recent head 6132b3b. Consider uploading reports for the commit 6132b3b to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ng/src/flight\_client\_scenarios/integration\_test.rs](https://codecov.io/gh/apache/arrow-rs/pull/1402/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aW50ZWdyYXRpb24tdGVzdGluZy9zcmMvZmxpZ2h0X2NsaWVudF9zY2VuYXJpb3MvaW50ZWdyYXRpb25fdGVzdC5ycw==) | `0.00% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1402/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (-0.46%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1402/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.43% <0.00%> (+0.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [afcf304...6132b3b](https://codecov.io/gh/apache/arrow-rs/pull/1402?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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