-
Notifications
You must be signed in to change notification settings - Fork 658
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
Make FlightSQL Support HTTPs #3388
Conversation
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.
Thank you @viirya
Ok(Self::new(channel)) | ||
} | ||
|
||
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server |
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.
Very cool 👍
@@ -59,6 +59,9 @@ jobs: | |||
- name: Test --examples | |||
run: | | |||
cargo test -p arrow-flight --features=flight-sql-experimental --examples | |||
- name: Test --examples with TLS |
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.
👨🍳 👌
Very nice -- thank you @viirya
let endpoint = endpoint | ||
.tls_config(tls_config) | ||
.map_err(|_| ArrowError::IoError("Cannot create endpoint".to_string()))?; | ||
|
||
let channel = endpoint.connect().await.map_err(|e| { |
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.
FWIW my eventual plan is to change the signature of the FlightSQL client to be fn new(channel: Channel)
(thus leaving the TLS configuration / setup up to the caller. It is great to have an example implementation / test in arrow-rs however
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.
Yea, this is basically similar signature as http version api. We can have a fn new(channel: Channel)
for more customized usage.
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 am going to propose (in some future PR) that instead of these higher level APIs we'll just have the Channel API and have doc examples showing how to configure the Channel for these usecases
Thank you @alamb |
Benchmark runs are scheduled for baseline = 17b3210 and contender = 733b7e7. 733b7e7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3309.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?