Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb merged PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb commented on PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#issuecomment-3079597911 Let's just get this one in Thanks again @mbrobbel -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb commented on code in PR #7839:
URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2205289302
##
arrow-flight/examples/flight_sql_server.rs:
##
@@ -814,7 +814,7 @@ mod tests {
async fn bind_tcp() -> (TcpIncoming, SocketAddr) {
let listener = TcpListener::bind("0.0.0.0:0").await.unwrap();
let addr = listener.local_addr().unwrap();
-let incoming = TcpIncoming::from_listener(listener, true,
None).unwrap();
+let incoming = TcpIncoming::from(listener);
Review Comment:
Thank you -- I fixed in
https://github.com/apache/arrow-rs/pull/7839/commits/f81914a0999c49249b014383477e5ec19c8380a2
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
MichaelScofield commented on code in PR #7839:
URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2199335122
##
arrow-flight/examples/flight_sql_server.rs:
##
@@ -814,7 +814,7 @@ mod tests {
async fn bind_tcp() -> (TcpIncoming, SocketAddr) {
let listener = TcpListener::bind("0.0.0.0:0").await.unwrap();
let addr = listener.local_addr().unwrap();
-let incoming = TcpIncoming::from_listener(listener, true,
None).unwrap();
+let incoming = TcpIncoming::from(listener);
Review Comment:
Previously the `TcpIncoming` in tonic 0.12 the `nodelay` was explicitly set
to true; however, here the default is `None`. Maybe it's better to align the
behavior.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb commented on PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#issuecomment-3058380515 @Xuanwo or @sundy-li or @carols10cents or @rmn-boiko or @MichaelScofield -- can someone let me know if this PR looks ok to you / would work in your use cases? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb commented on code in PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2183629672 ## arrow-flight/Cargo.toml: ## @@ -64,7 +64,7 @@ default = [] flight-sql = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] # TODO: Remove in the next release flight-sql-experimental = ["flight-sql"] -tls = ["tonic/tls"] +tls = ["tonic/tls-ring"] Review Comment: Actually, I see there are examples and binaries that want tls > warning: invalid feature `tls` in required-features of target `flight_sql_client`: `tls` is not present in [features] section I will then follow the pattern and rename the tls flags to match tonic -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb commented on code in PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2183626296 ## arrow-flight/Cargo.toml: ## @@ -64,7 +64,7 @@ default = [] flight-sql = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] # TODO: Remove in the next release flight-sql-experimental = ["flight-sql"] -tls = ["tonic/tls"] +tls = ["tonic/tls-ring"] Review Comment: I think adding nothing makes the most sense to me -- and let the user choose with their own feature. I will remove 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
carols10cents commented on code in PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2183609337 ## arrow-flight/Cargo.toml: ## @@ -64,7 +64,7 @@ default = [] flight-sql = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] # TODO: Remove in the next release flight-sql-experimental = ["flight-sql"] -tls = ["tonic/tls"] +tls = ["tonic/tls-ring"] Review Comment: I _think_ if you add this, ~~then users won't be able to choose "tonic/tls-aws-lc" because they're mutually exclusive~~ nevermind, this is off by default. This would just be inconsistent and possibly confusing to only have a feature that turns on `tonic/tls-ring` but not `tonic/tls-aws-lc`, and someone using `tonic/tls-aws-lc` might go to turn on `arrow-flight/tls` and have a bad time. So either arrow-flight should add: ``` tls-ring = ["tonic/tls-ring"] tls-aws-lc = ["tonic/tls-aws-lc"] ``` or it should add nothing and assume feature unification from the user of `arrow-flight` also using `tonic` and configuring the feature with their use of `tonic` will take care of 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
carols10cents commented on code in PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2183609337 ## arrow-flight/Cargo.toml: ## @@ -64,7 +64,7 @@ default = [] flight-sql = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] # TODO: Remove in the next release flight-sql-experimental = ["flight-sql"] -tls = ["tonic/tls"] +tls = ["tonic/tls-ring"] Review Comment: I _think_ if you add this, ~~then users won't be able to choose "tonic/tls-aws-lc" because they're mutually exclusive~~ nevermind, this is off by default. This would just be inconsistent and possibly confusing to only have a feature that turns on tonic/tls-ring but not tonic/tls-aws-lc, and someone using tonic/tls-aws-lc might go to turn on arrow-flight/tls and have a bad time. So either arrow-flight should add: ``` tls-ring = ["tonic/tls-ring"] tls-aws-lc = ["tonic/tls-aws-lc"] ``` or it should add nothing and assume feature unification from the user of arrow-flight also using tonic and configuring the feature with their use of tonic will take care of 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
carols10cents commented on code in PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2183609337 ## arrow-flight/Cargo.toml: ## @@ -64,7 +64,7 @@ default = [] flight-sql = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] # TODO: Remove in the next release flight-sql-experimental = ["flight-sql"] -tls = ["tonic/tls"] +tls = ["tonic/tls-ring"] Review Comment: I _think_ if you add this, then users won't be able to choose "tonic/tls-aws-lc" because they're mutually exclusive. So either arrow-flight should add: ``` tls-ring = ["tonic/tls-ring"] tls-aws-lc = ["tonic/tls-aws-lc"] ``` or it should add nothing and assume feature unification from the user of arrow-flight also using tonic and configuring the feature with their use of tonic will take care of 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb commented on code in PR #7839: URL: https://github.com/apache/arrow-rs/pull/7839#discussion_r2177362124 ## arrow-flight/Cargo.toml: ## @@ -64,7 +64,7 @@ default = [] flight-sql = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] # TODO: Remove in the next release flight-sql-experimental = ["flight-sql"] -tls = ["tonic/tls"] +tls = ["tonic/tls-ring"] Review Comment: I am not sure what the implications of this change are. @Xuanwo or @sundy-li can you help me figure out if this is the right solution or not? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] Upgrade tonic dependencies to 0.13.0 version (try 2) [arrow-rs]
alamb opened a new pull request, #7839: URL: https://github.com/apache/arrow-rs/pull/7839 # Which issue does this PR close? - Related to #7395 - Closes https://github.com/apache/arrow-rs/pull/7495 - Closes https://github.com/apache/arrow-rs/pull/7377 # Rationale for this change Let's update tonic to the latest Given the open and unresolved questions on @rmn-boiko's PR https://github.com/apache/arrow-rs/pull/7377 from @Xuanwo and @sundy-li, I thought a new PR would result in a faster resolution. # What changes are included in this PR? This PR is based on https://github.com/apache/arrow-rs/pull/7495 from @MichaelScofield -- I resolved some merge conflicts and updated Cargo.toml in the integration tests # Are these changes tested? Yes, by CI # Are there any user-facing changes? New dependency version -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
