You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2023/03/02 13:06:55 UTC
[GitHub] [arrow-rs] crepererum opened a new pull request, #3788: refactor: assorted `FlightSqlServiceClient` improvements
crepererum opened a new pull request, #3788:
URL: https://github.com/apache/arrow-rs/pull/3788
# Which issue does this PR close?
\-
# Rationale for this change
Assorted `FlightSqlServiceClient` improvements:
- **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
# What changes are included in this PR?
See list above.
# Are there any user-facing changes?
**Breaking** slight changes in the `FlightSqlServiceClient` interface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1123286275
##########
arrow-flight/src/sql/client.rs:
##########
@@ -83,13 +75,23 @@ impl FlightSqlServiceClient {
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server
#[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
+ pub async fn new_with_tls_endpoint(
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#issuecomment-1453406714
Benchmark runs are scheduled for baseline = b9fcd7fa2154f848823521a11aeeba4e687025b8 and contender = 40e2874e1d83dd8dc64981b7f4a19f894befe615. 40e2874e1d83dd8dc64981b7f4a19f894befe615 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/157a70544469471394767265ab184a4d...7437ac51b1004007a4de6c5bfe3650ab/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e79101a5571a4e89b0ab43d45dd79e67...6ca52d9b10bb4113b3646bc254c90c11/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/5e6ae60884184cc29da1d4bb82048d44...28a82666a4f34915a06d9bd4b9136ab6/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/9bc026d75de3468c9888460b68a4cf85...5e50e1b365a64af7b7d1aab96e4e2cc3/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1123267191
##########
arrow-flight/src/sql/client.rs:
##########
@@ -83,13 +75,23 @@ impl FlightSqlServiceClient {
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server
#[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
+ pub async fn new_with_tls_endpoint(
Review 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 ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] crepererum commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1123475769
##########
arrow-flight/src/sql/client.rs:
##########
@@ -83,13 +75,23 @@ impl FlightSqlServiceClient {
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server
#[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
+ pub async fn new_with_tls_endpoint(
Review Comment:
I can change that tomorrow morning (central EU).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] crepererum commented on pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#issuecomment-1453319826
Windows build fails due to https://github.com/rust-lang/socket2/issues/408
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] crepererum commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1123256026
##########
arrow-flight/src/sql/client.rs:
##########
@@ -83,13 +75,23 @@ impl FlightSqlServiceClient {
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server
#[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
+ pub async fn new_with_tls_endpoint(
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1125020931
##########
arrow-flight/src/sql/client.rs:
##########
@@ -44,85 +45,29 @@ use arrow_ipc::{root_as_message, MessageHeader};
use arrow_schema::{ArrowError, Schema, SchemaRef};
use futures::{stream, TryStreamExt};
use prost::Message;
-#[cfg(feature = "tls")]
-use tonic::transport::{Certificate, ClientTlsConfig, Identity};
-use tonic::transport::{Channel, Endpoint};
-use tonic::Streaming;
+use tonic::transport::Channel;
+use tonic::{IntoRequest, Streaming};
/// A FlightSQLServiceClient is an endpoint for retrieving or storing Arrow data
/// by FlightSQL protocol.
#[derive(Debug, Clone)]
pub struct FlightSqlServiceClient {
token: Option<String>,
+ headers: HashMap<String, String>,
flight_client: FlightServiceClient<Channel>,
}
/// A FlightSql protocol client that can run queries against FlightSql servers
/// This client is in the "experimental" stage. It is not guaranteed to follow the spec in all instances.
/// Github issues are welcomed.
impl FlightSqlServiceClient {
- /// Creates a new FlightSql Client that connects via TCP to a server
- #[cfg(not(feature = "tls"))]
- pub async fn new_with_endpoint(host: &str, port: u16) -> Result<Self, ArrowError> {
- let addr = format!("http://{}:{}", host, port);
- let endpoint = Endpoint::new(addr)
- .map_err(|_| ArrowError::IoError("Cannot create endpoint".to_string()))?
- .connect_timeout(Duration::from_secs(20))
- .timeout(Duration::from_secs(20))
- .tcp_nodelay(true) // Disable Nagle's Algorithm since we don't want packets to wait
- .tcp_keepalive(Option::Some(Duration::from_secs(3600)))
- .http2_keep_alive_interval(Duration::from_secs(300))
- .keep_alive_timeout(Duration::from_secs(20))
- .keep_alive_while_idle(true);
-
- let channel = endpoint.connect().await.map_err(|e| {
- ArrowError::IoError(format!("Cannot connect to endpoint: {}", e))
- })?;
- Ok(Self::new(channel))
- }
-
- /// Creates a new HTTPs FlightSql Client that connects via TCP to a server
- #[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
- host: &str,
- port: u16,
- ) -> Result<Self, ArrowError> {
- let addr = format!("https://{host}:{port}");
-
- let endpoint = Endpoint::new(addr)
- .map_err(|_| ArrowError::IoError("Cannot create endpoint".to_string()))?
- .connect_timeout(Duration::from_secs(20))
- .timeout(Duration::from_secs(20))
- .tcp_nodelay(true) // Disable Nagle's Algorithm since we don't want packets to wait
- .tcp_keepalive(Option::Some(Duration::from_secs(3600)))
- .http2_keep_alive_interval(Duration::from_secs(300))
- .keep_alive_timeout(Duration::from_secs(20))
- .keep_alive_while_idle(true);
-
- let tls_config = ClientTlsConfig::new()
- .domain_name(domain)
- .ca_certificate(server_ca)
- .identity(client_ident);
-
- let endpoint = endpoint
- .tls_config(tls_config)
- .map_err(|_| ArrowError::IoError("Cannot create endpoint".to_string()))?;
-
- let channel = endpoint.connect().await.map_err(|e| {
- ArrowError::IoError(format!("Cannot connect to endpoint: {e}"))
- })?;
- 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 {
Review Comment:
❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1123226359
##########
arrow-flight/src/sql/client.rs:
##########
@@ -83,13 +75,23 @@ impl FlightSqlServiceClient {
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server
#[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
+ pub async fn new_with_tls_endpoint(
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] avantgardnerio commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1123379913
##########
arrow-flight/src/sql/client.rs:
##########
@@ -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);
Review Comment:
So this will then propagate to all calls? :heart_eyes:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] crepererum commented on a diff in pull request #3788: refactor: assorted `FlightSqlServiceClient` improvements
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #3788:
URL: https://github.com/apache/arrow-rs/pull/3788#discussion_r1124252065
##########
arrow-flight/src/sql/client.rs:
##########
@@ -83,13 +75,23 @@ impl FlightSqlServiceClient {
/// Creates a new HTTPs FlightSql Client that connects via TCP to a server
#[cfg(feature = "tls")]
- pub async fn new_with_endpoint(
- client_ident: Identity,
- server_ca: Certificate,
- domain: &str,
+ pub async fn new_with_tls_endpoint(
Review Comment:
done (also simplified example and CI around it)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org