ARROW-8853: [Rust] [Integration Testing] Enable Flight tests #9049
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carols10cents
force-pushed
the
rust-flight-integration
branch
from
ce2519b
to
2c5899c
Compare
alamb
approved these changes
Dec 31, 2020
There was a problem hiding this comment.
I reviewed this code commit by commit (thanks @carols10cents for splitting it up into a easy-to-follow chain).
While I am not an expert in Arrow Flight, I did find the code easy to follow and understand and it made sense to me.
As predicted, some java tests failed on this integration test and I have pinged some people on #8963 and hopefully we can get that merged soon. The error was
RuntimeError: Command failed: java -Dio.netty.tryReflectionSetAccessible=true -Darrow.struct.conflict.policy=CONFLICT_APPEND -cp /arrow/java/flight/flight-core/target/flight-core-3.0.0-SNAPSHOT-jar-with-dependencies.jar org.apache.arrow.flight.example.integration.IntegrationTestClient -port 39459 -j /tmp/arrow-integration-xu49kq36/generated_null_trivial.json
With output:
--------------
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Verifying location grpc+tcp://0.0.0.0:39459
Unknown error
java.lang.IllegalArgumentException: A batch can only be consumed if it contains a single ArrowBuf.
When I looked at the rest of the logs, I see the rust producer is running and passing. 鉂わ笍 鉂わ笍
2020-12-30T20:04:00.8176245Z ##########################################################
2020-12-30T20:04:00.8176859Z IPC: C++ producing, Rust consuming
2020-12-30T20:04:00.8177378Z ##########################################################
2020-12-30T20:05:45.7634027Z ##########################################################
2020-12-30T20:05:45.7634638Z IPC: Java producing, Rust consuming
2020-12-30T20:05:45.7635156Z ##########################################################
And there are others too.
Really nice work 馃帀
|
The full set of Rust CI tests did not run on this PR :( Can you please rebase this PR against apache/master to pick up the changes in #9056 so that they do? I apologize for the inconvenience. |
carols10cents
force-pushed
the
rust-flight-integration
branch
from
2c5899c
to
84d9b72
Compare
|
No problem at all, rebased! Thank you for the review 鉂わ笍 |
Codecov Report
@@ Coverage Diff @@
## master #9049 +/- ##
==========================================
- Coverage 82.57% 81.81% -0.77%
==========================================
Files 204 214 +10
Lines 50879 51352 +473
==========================================
- Hits 42014 42013 -1
- Misses 8865 9339 +474
Continue to review full report at Codecov.
|
|
@carols10cents this needs a rebase :) |
nevi-me
approved these changes
Jan 7, 2021
|
@carols10cents -- #8963 was just merged - so hopefully after a rebase this PR will be ready too |
carols10cents
force-pushed
the
rust-flight-integration
branch
from
84d9b72
to
a36694b
Compare
This function returns, potentially, a few dictionaries, and will always return one record batch. Return these as a tuple rather than putting them together in one vec to clarify.
This doesn't seem necessary
Tracked in ARROW-10961. There's a bug in tonic that doesn't handle headers and trailers correctly; it has been fixed but a new version of tonic needs to be released and used to get the fix.
carols10cents
force-pushed
the
rust-flight-integration
branch
from
a36694b
to
863420d
Compare
|
Rebased, and CI is looking good! |
|
馃帀 |
GeorgeAp
pushed a commit
to sirensolutions/arrow
that referenced
this pull request
Jun 7, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 馃帀 The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961). Some Rust <-> Java integration tests will fail until [this PR is merged](apache#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!! Closes apache#9049 from carols10cents/rust-flight-integration Lead-authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com> Co-authored-by: Jake Goulding <jake.goulding@gmail.com> Signed-off-by: Neville Dipale <nevilledips@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 馃帀
The middleware scenario tests are currently skipped because they will fail until
toniccan be updated to a version containing a fix having to do with trailers; this is tracked in ARROW-10961.Some Rust <-> Java integration tests will fail until this PR is merged; I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!!