Skip to content
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

Feature gate arrow-flight client #5683

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

leoyvens
Copy link

The tonic/transport tonic feature, which has large dependencies like hyper and axum, is now feature gated by a client feature on the arrow-flight crate. This allows users that only want the flight server to avoid dependency bloat.

Which issue does this PR close?

Closes #5682.

Are there any user-facing changes?

This will break users who are using the client. The fix is for them to specify the new feature flag. I did not include the client feature in the default features, but I have no particular opinion there.

The `tonic/transport` feature, which has
large dependencies like hyper and axum, is gated by this.
@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Apr 23, 2024
@tustvold tustvold added the api-change Changes to the arrow API label Apr 23, 2024
@tustvold
Copy link
Contributor

I'm not against this, but just as an FYI it will need to wait for the next breaking release

@tustvold
Copy link
Contributor

Taking another look at this I'm confused, transport is required both for clients and servers. Axum in particular is a dependency of the server implementation?

@leoyvens
Copy link
Author

Yes, in tonic, the transport feature is required for the "batteries included" server. But arrow-rs does not currently expose a high-level server API that depends on the tonic Server. Users need to choose their http2 server, for which they could use the tonic transport::server, or not.

@tustvold
Copy link
Contributor

tustvold commented Apr 24, 2024

My understanding of the tonic traits was that they were so low level that whilst in theory one could plugin an alternative this wasn't really practical. I'm still confused why this feature flag is called client when AFAICT it is required for servers and clients?

The reason behind my hesitation is feature flags are a pain to maintain and test, so we only want to add them where there is real pain

@leoyvens
Copy link
Author

leoyvens commented Apr 24, 2024

For arrow-flight it is truly only required for the high-level client, not for the server since FlightServiceServer is just a Service trait implementation. But I'm happy to rename the feature gate.

My use-case would be to build a simple server by using hyper directly, without axum or the tonic server. I wanted to use hyper 1.0 without waiting for tonic to upgrade, but turns out this isn't possible due to conflicting http crate versions between hyper 1.0 and tonic.

That is to say, I have not been able to get my use case for this PR actually working. And once tonic upgrades to hyper 1.0 and http 1.0, I suppose the only unneeded dependency would be axum itself, and I wouldn't classify that as a 'real pain'. So I think it's fair if you'd rather not do this at this point.

@tustvold
Copy link
Contributor

Heh, I figured something like that might be the case having gone down much the same rabbit hole myself, hence my question.

Given that I suggest we keep this as a draft and revisit if tonic continues to not upgrade http.

@tustvold tustvold marked this pull request as draft April 26, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature gate arrow flight client
2 participants