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

Re-enable C# arrow flight integration test #6611

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 21, 2024

Which issue does this PR close?

Closes #6577

Rationale for this change

This test was turned off in #6598 as it was failing

What changes are included in this PR?

Implement @adamreeve 's suggestion (❤️ ) from apache/arrow#44377 (comment)

  1. Change address sent in FlightInfo to be localhost rather than 0.0.0.0
  2. Re-enable the integration test

Note that @adamreeve says this might be something the C# client should handle, so maybe we should take a different approach 🤔

But I'm not 100% sure this isn't something the C# client should handle, given the other clients seem OK with it.

Are there any user-facing changes?

No this is testing only

@alamb alamb marked this pull request as ready for review October 21, 2024 15:47
@alamb alamb marked this pull request as draft October 21, 2024 15:47
@github-actions github-actions bot added the arrow Changes to the arrow crate label Oct 21, 2024
@alamb alamb marked this pull request as ready for review October 21, 2024 16:06

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 🤔

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:

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

@alamb
Copy link
Contributor Author

alamb commented Oct 24, 2024

Let's go with this solution and revisit if anyone else has issues. As this is all a test harness I don't think this has any implications for actual Flight implementations

@alamb alamb merged commit 1103939 into apache:master Oct 24, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Archery Integration Test with c# failing on main
2 participants