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

Fallible stream for arrow-flight do_exchange call (#3462) #5698

Merged
merged 1 commit into from
May 2, 2024

Conversation

opensourcegeek
Copy link
Contributor

Which issue does this PR close?

Closes #3462

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Apr 27, 2024
@opensourcegeek opensourcegeek marked this pull request as ready for review April 27, 2024 16:58
@opensourcegeek opensourcegeek force-pushed the feat/3462 branch 2 times, most recently from f9ec823 to 8fe8a63 Compare April 27, 2024 20:08
@alamb alamb added the api-change Changes to the arrow API label Apr 30, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thank you @opensourcegeek 🙏

Since this is an API change I think it could be merged when we are ready for the next release with breaking API changes

&mut self,
request: S,
) -> Result<FlightRecordBatchStream> {
let request = self.make_request(request);
let (sender, mut receiver) = futures::channel::oneshot::channel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is some way to avoid the repetition here with the code in do_get above 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did wonder about duplication of the code as in do_put method, making all errors (both in request stream and response stream) go back to client. Since it's my first MR here I didn't want to take it further. But happy to explore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could do it as a follow on PR 🤔

I think this PR is ready to merge as long as we can accept API changes in this release (we are working to slow down the rate of breaking API changes - see #5368 )

@tustvold tustvold merged commit eb2d00b into apache:master May 2, 2024
12 checks passed
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.

Flight Client should accept fallible streams (do_put and do_exchange)
4 participants