-
Notifications
You must be signed in to change notification settings - Fork 861
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
Better flight SQL example codes #4144
Conversation
I profiled the test case
|
Oh, it's due to TcpNoDelay, it's not enabled. databendlabs/databend#11252 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, mostly just some minor nits. I took the liberty of creating #4145 which should remove the need for the once_cell shenanigans. Happy to update this PR once that is in.
{ | ||
let addr = "127.0.0.1:50051"; | ||
|
||
// We would just listen on TCP, but it seems impossible to know when tonic is ready to serve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// We would just listen on TCP, but it seems impossible to know when tonic is ready to serve |
I believe the sleep below is to handle this
.serve(addr.parse().unwrap()); | ||
|
||
let request_future = async { | ||
sleep(Duration::from_millis(2000)).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sleep(Duration::from_millis(2000)).await; | |
// wait for server to start up | |
sleep(Duration::from_millis(2000)).await; |
F: FnOnce(FlightSqlServiceClient<Channel>) -> C, | ||
C: Future<Output = ()>, | ||
{ | ||
let addr = "127.0.0.1:50051"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4145 shows how this could be adapted to not need to hard-code a port
arrow-flight/Cargo.toml
Outdated
@@ -34,17 +34,37 @@ arrow-cast = { workspace = true } | |||
arrow-ipc = { workspace = true } | |||
arrow-schema = { workspace = true } | |||
base64 = { version = "0.21", default-features = false, features = ["std"] } | |||
tonic = { version = "0.9", default-features = false, features = ["transport", "codegen", "prost"] } | |||
tonic = { version = "0.9", default-features = false, features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find this formatting harder to read...
Working on updating this |
Thank you for this 👍 |
Which issue does this PR close?
Closes #.
Rationale for this change
Provide more detail examples and test codes for flight SQL.
What changes are included in this PR?
Are there any user-facing changes?