GH-44361: [C#][Integration] Include .NET in Flight integration tests #44377
Conversation
Specify locations in GetFlightInfo using the correct scheme, and handle creating GRPC channels using this scheme.
adamreeve
requested review from
assignUser,
jonkeane,
kou,
raulcd and
CurtHagenlocher
as code owners
| @@ -64,7 +64,7 @@ protected virtual void Dispose(bool disposing) | |||
| { | |||
| if (!_disposed) | |||
| { | |||
| _flightDataStream.Dispose(); | |||
| _flightDataStream?.Dispose(); | |||
There was a problem hiding this comment.
This isn't required for any of the included tests, but before disabling the primitive_no_batches test, it would crash here with a NullReferenceException due to the writer being disposed before the stream was created.
With this fix, writing doesn't crash, but the data isn't found when trying to retrieve it.
github-actions
bot
added
awaiting committer review
|
This is awesome; I didn't even know this test gap existed. By the way, I have a proposed fix for #38045 at https://github.com/CurtHagenlocher/arrow/tree/dev/curth/FlightDictionaries which I didn't feel great about committing due to lack of test coverage. It'll be interesting to see whether it works. |
|
There was a failure running the new tests in CI. Surprisingly it's a C++ and C# test, which was the only combination I tested locally... I'll try to figure out what's gone wrong here: |
Cool, it would be nice to fix that! |
|
There was a race condition where the C# client could try to request the data before the server stored it here: I could reliably reproduce it by adding a small sleep in the C++ server before that line. Reading to the end of the response stream in the client seems to fix it. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 1dcd145. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
|
Hi @adamreeve -- thank you for adding this test. This test unfortunately fails in arrow-rs -- see apache/arrow-rs#6577 Do you have any hints about how we can potentially fix this? Here is how the job is run: https://github.com/apache/arrow-rs/blob/1666a4d7b237370c167e2b90b45d1354e28f500a/.github/workflows/integration.yml#L50-L114 Here is a specific message from the logs https://github.com/apache/arrow-rs/actions/runs/11409436304/job/31749661931 |
|
Hi @alamb, sorry about this, it looks like in I can reproduce this locally and can see that when the C# client makes a get_info request to the Rust server, the Rust server sends a location like "grpc+tcp://0.0.0.0:38119". In comparison, the C++ server for example returns locations that use the ipv4 loopback address, eg. "grpc+tcp://127.0.0.1:43861". This looks like a bug in the Rust integration test server to me, as 0.0.0.0 is valid to use as an IP address when binding to a port as a server, but as the .NET exception says, this is an "unspecified address that cannot be used as a target address". The Rust server should specify a public address that clients can connect to, or return an empty list of locations to specify that the current connection can be reused (from the Flight RPC docs). I'm not sure why other clients don't have a problem with this though. It seems like the C++ client is quite happy to connect to a "0.0.0.0" address for example, which is a little surprising to me, so maybe I'm misunderstanding this. |
Oh, sorry. It's a bug of #44099. Could you open an issue for this? |
|
This change in arrow-rs fixes the tests for me: --- a/arrow-integration-testing/src/flight_server_scenarios/integration_test.rs
+++ b/arrow-integration-testing/src/flight_server_scenarios/integration_test.rs
@@ -48,9 +48,10 @@ type Result<T = (), E = Error> = std::result::Result<T, E>;
/// Run a scenario that tests integration testing.
pub async fn scenario_setup(port: u16) -> Result {
let addr = super::listen_on(port).await?;
+ let resolved_port = addr.port();
let service = FlightServiceImpl {
- server_location: format!("grpc+tcp://{addr}"),
+ server_location: format!("grpc+tcp://localhost:{resolved_port}"),
..Default::default()
};
let svc = FlightServiceServer::new(service);
But I'm not 100% sure this isn't something the C# client should handle, given the other clients seem OK with it. |
Thank you 🙏 @adamreeve I made a PR to try out your proposed workaround in arrow-rs: apache/arrow-rs#6611 |
|
I'm relatively unfamiliar with Flight in general (to say nothing of any given implementation) but I have to wonder whether this reflects a product bug in the C# code. If I connect to a server and ask it for information and it tells me "find that information at "grpc+tcp://0.0.0.0:38119", then my assumption would be that I'm meant to connect to an endpoint on the same server that I originally connected to. |
|
The Flight docs specify other ways to reuse the same server location though, either using an empty list of locations or the From looking into this a bit more, it seems like using 0.0.0.0 to refer to localhost/loopback when used as a target IP is non-standard but not that uncommon, and the C# Given that this would be fixed in either the Rust or C# integration test code without changing the Flight libraries, I guess it's not too consequential which approach is used, and I'm happy to update the C# integration test client to handle this. But the integration test code may be used by users as an example of what to do, so it seems like changing the Rust server might be best to set a good example of what to do to provide a more compatible server. |
This PR proposes to do that: apache/arrow-rs#6611 However, @tustvold is still helping me get the incantation just right (see apache/arrow-rs#6611 (comment)) |
After some more thought I agree that changing the C# code to handle Thanks again for your help |
…44377) ### Rationale for this change See #44361. This allows testing compatibility of the .NET Flight implementation with other Flight implementations. ### What changes are included in this PR? * Adds a new `Apache.Arrow.Flight.IntegrationTest` project that can run in server or client mode for Flight integration tests. * Includes the integration tests that send then retrieve data defined in JSON files, but doesn't add any of the named scenarios * Configures archery to include C# in the Flight integration tests, but skip all the named scenarios * Also skips tests that use dictionary data due to #38045, and the empty data test due to #44363 ### Are these changes tested? These changes are tests. ### Are there any user-facing changes? No * GitHub Issue: #44361 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
Rationale for this change
See #44361. This allows testing compatibility of the .NET Flight implementation with other Flight implementations.
What changes are included in this PR?
Apache.Arrow.Flight.IntegrationTestproject that can run in server or client mode for Flight integration tests.Are these changes tested?
These changes are tests.
Are there any user-facing changes?
No