refactor: assorted FlightSqlServiceClient improvements
#3788
Conversation
github-actions
bot
added
arrow
- **TLS config:** Do NOT alter existing method signatures if the TLS
feature is enabled. Features should be purely additive in Rust.
Instead use a new method to pass TLS configs. The config is now passed
as `ClientTlsConfig` to allow more flexibility, e.g. just to use TLS
w/o any client certs.
- **token handlng:** Allow the token to be passed in from an external
source. The [auth spec] is super flexibility ("application-defined")
and we cannot derive a way to determine the token in all cases. The
current handshake-based mechanism is OK though. Also make sure the
token is used in all relevant methods.
- **headers:** Allow users to pass in additional headers. This is
helpful for certain applications.
[auth spec]: https://arrow.apache.org/docs/format/Flight.html#authentication
crepererum
force-pushed
the
crepererum/flight_sql_client_improvements
branch
from
9c38019 to
8a118bd
Compare
There was a problem hiding this comment.
I think this is an improvement -- thank you @crepererum . However, I think it would be good if @avantgardnerio could offer his opinions too prior to merge
arrow-flight/src/sql/client.rs
Outdated
| client_ident: Identity, | ||
| server_ca: Certificate, | ||
| domain: &str, | ||
| pub async fn new_with_tls_endpoint( |
There was a problem hiding this comment.
what do you think about removing all tls configuration out of the actual client and instead add a doc example that shows how to configure the client for TLS?
I feel like simply having new(channel: Channel) would be the simplest API and all the rest of the code in this function could be turned into doc comments. This would likely be simpler to use, have less code, and be easier to configure with arbitrary configurations (like keepalives, etc)
There was a problem hiding this comment.
At the moment we also use a very opinionated Endpoint config (see fn endpoint). If we keep that then I would at least like to offer a way to configure a TLS client that is in line with that presets. However we may also just remove the opinionated endpoint setup altogether and ONLY present a single constructor that takes channel. WDYT?
There was a problem hiding this comment.
That is what I would recommend and what I started to do in https://github.com/apache/arrow-rs/pull/3401/files#diff-f9c519f51ac059c26fa8fb28b174ebf8d66169690758b6b8a53bcf13ec6acb58R132
Let me see if I can update that PR to get it looking better ...
There was a problem hiding this comment.
(that PR is too far gone, so I closed it). But I think the idea is still a good one (take only an Endpoint) -- and it is consistent with the FlightClient: https://docs.rs/arrow-flight/34.0.0/arrow_flight/client/struct.FlightClient.html#method.new
There was a problem hiding this comment.
I can change that tomorrow morning (central EU).
There was a problem hiding this comment.
done (also simplified example and CI around it)
| @@ -141,14 +132,27 @@ impl FlightSqlServiceClient { | |||
| self.flight_client | |||
| } | |||
|
|
|||
| /// Set auth token to the given value. | |||
| pub fn set_token(&mut self, token: String) { | |||
| self.token = Some(token); | |||
There was a problem hiding this comment.
So this will then propagate to all calls? 😍
|
Windows build fails due to rust-lang/socket2#408 Update: version was yanked, re-pushed to retrigger the build. |
Just accept a channel and let the caller set it up to their liking. Simplify example as well so that we no longer do totally different things under different features (since features shall be additive). Instead use a single example.
crepererum
force-pushed
the
crepererum/flight_sql_client_improvements
branch
from
ea826b1 to
fe36584
Compare
|
Benchmark runs are scheduled for baseline = b9fcd7f and contender = 40e2874. 40e2874 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
| })?; | ||
| Ok(Self::new(channel)) | ||
| } | ||
|
|
||
| /// Creates a new FlightSql client that connects to a server over an arbitrary tonic `Channel` | ||
| pub fn new(channel: Channel) -> Self { |
Which issue does this PR close?
-
Rationale for this change
Assorted
FlightSqlServiceClientimprovements:ClientTlsConfigto allow more flexibility, e.g. just to use TLS w/o any client certs.What changes are included in this PR?
See list above.
Are there any user-facing changes?
Breaking slight changes in the
FlightSqlServiceClientinterface.