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.

Sign up for GitHub

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

Re-enable C# arrow flight integration test #6611

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Re-enable C# arrow flight integration test #6611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ jobs:
ARROW_RUST_EXE_PATH: /build/rust/debug
BUILD_DOCS_CPP: OFF
ARROW_INTEGRATION_CPP: ON
# Disable C# integration tests due to https://github.com/apache/arrow-rs/issues/6577
ARROW_INTEGRATION_CSHARP: OFF
ARROW_INTEGRATION_CSHARP: ON
ARROW_INTEGRATION_GO: ON
ARROW_INTEGRATION_JAVA: ON
ARROW_INTEGRATION_JS: ON
Expand Down
6 changes: 5 additions & 1 deletion arrow-integration-testing/src/flight_server_scenarios/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,13 @@ 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}"),
// See https://github.com/apache/arrow-rs/issues/6577
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CurtHagenlocher also mentions maybe the C# should be changed rather than the rust test harness: apache/arrow#44377 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW listen_on should probably be returning the resolved socket address.

Also localhost may not work on machines with dual stack IPv4 and IPv6, as the latter will be preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know how to get the resolved address? I tried addr.ip() but that still returns the unresolved port.

#[tokio::main]
async fn main() -> Result<(), DataFusionError> {
    let port = 9999;
    let addr: SocketAddr = format!("0.0.0.0:{port}").parse().unwrap();

    let listener = TcpListener::bind(addr).unwrap();
    let addr = listener.local_addr().unwrap();
    println!("Listening on: {}", addr);
    println!("Resolved host: {}", addr.ip().to_string());
    println!("Resolved port: {}", addr.port());

    Ok(())
}

Which prints out

Listening on: 0.0.0.0:9999
Resolved host: 0.0.0.0
Resolved port: 9999

I think only the actual Server knows the port after calling listen, but the tower/tonic thing only returns some sort of future (not a server that can be interrogated) 🤔

    let server = Server::builder().add_service(svc).serve(addr);

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC you bind the TCPListener - https://docs.rs/tokio/1.40.0/tokio/net/struct.TcpListener.html, and get the address of it https://docs.rs/tokio/1.40.0/tokio/net/struct.TcpListener.html#method.local_addr

You can then construct a https://docs.rs/tonic/latest/tonic/transport/server/struct.TcpIncoming.html and feed this to the tonic builder.

It's a bit convoluted, but it is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my simple example from above does something like this (without the TcpIncoming) (binds to 0.0.0.0 and then gets local_addr) but that still seems to return 0.0.0.0

    let addr: SocketAddr = format!("0.0.0.0:{port}").parse().unwrap();

    let listener = TcpListener::bind(addr).unwrap();
    let addr = listener.local_addr().unwrap();

It seems like this is what listen_on also tries to do 🤔

arrow-rs/arrow-integration-testing/src/flight_server_scenarios/mod.rs

Lines 33 to 38 in 7781bc2

let addr: SocketAddr = format!("0.0.0.0:{port}").parse()?;
let listener = TcpListener::bind(addr).await?;
let addr = listener.local_addr()?;
Ok(addr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like other places in the tests also use localhost as the hostname here as well:

arrow-rs/arrow-integration-testing/src/flight_server_scenarios/middleware.rs

Line 103 in 7781bc2

let endpoint = super::endpoint("foo", "grpc+tcp://localhost:10010");

So maybe this is a problem that we can address if someone has issues running the tests on their environment. It "works for me" locally and it works on CI so perhaps that is good enough

I'll wait a while longer to see if anyone else has any good ideas

// C# had trouble resolving addressed like 0.0.0.0:port
// server_location: format!("grpc+tcp://{addr}"),
server_location: format!("grpc+tcp://localhost:{resolved_port}"),
..Default::default()
};
let svc = FlightServiceServer::new(service);
Expand Down
Loading