Refactor to share code between do_put and do_exchange calls #5728
Conversation
github-actions
bot
added
arrow
arrow-flight/src/client.rs
Outdated
| @@ -704,3 +679,75 @@ impl FlightClient { | |||
| request | |||
| } | |||
| } | |||
|
|
|||
| struct FallibleRequestStream { | |||
There was a problem hiding this comment.
looks good to me 👍
Perhaps we could add some comments about what this struct is doing and what it is for.
There was a problem hiding this comment.
I did refactor it so that this is generic and not tied to request stream itself, is it worth moving this bit of code to some internal_utils module? Happy to leave it where it is atm
arrow-flight/src/client.rs
Outdated
| fn poll_next(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Option<Self::Item>> { | ||
| let pinned = self.get_mut(); | ||
| let mut request_streams = pinned.request_streams.as_mut(); | ||
| match request_streams.poll_next_unpin(cx) { |
There was a problem hiding this comment.
I think you can avoid a level of nesting using the ready! macro here
so like
match ready!(request_streams.poll_next_unpin(cx) {
Some(Ok(data)) => ...
Some(Err(e)) => ...
None => ...
}There was a problem hiding this comment.
I did expand the macro from original code for my own understanding, I'll tidy it up. Thanks
arrow-flight/src/client.rs
Outdated
| return Poll::Ready(Some(Err(err))); | ||
| }; | ||
|
|
||
| match pinned.response_streams.poll_next_unpin(cx) { |
There was a problem hiding this comment.
Likewise here, calling ready!() can probably simplify this a bit
opensourcegeek
force-pushed
the
feat/3462-refactor
branch
3 times, most recently
from
6a8fc13 to
3806b5a
Compare
opensourcegeek
force-pushed
the
feat/3462-refactor
branch
from
3806b5a to
7729b20
Compare
Signed-off-by: Praveen Kumar <[email protected]>
opensourcegeek
force-pushed
the
feat/3462-refactor
branch
from
7729b20 to
9b8b42b
Compare
There was a problem hiding this comment.
Thanks @opensourcegeek -- I think this is a very nice improvement to me. 🙏
| match ready!(request_streams.poll_next_unpin(cx)) { | ||
| Some(Ok(data)) => Poll::Ready(Some(data)), | ||
| Some(Err(e)) => { | ||
| // unwrap() here is safe, ownership of sender will |
There was a problem hiding this comment.
Another potential improvement might be to do something like
if let Some(sender) = pinned.sender.take() {
sender.send();
}And that way avoid a panic
However, I see this unwrap is simply moved here from elsewhere in the PR so I think we can improve it in the future
There was a problem hiding this comment.
Ah good point, it would've taken less time to swap the code than writing the justification comments.
| @@ -704,3 +674,99 @@ impl FlightClient { | |||
| request | |||
| } | |||
| } | |||
|
|
|||
| /// Wrapper around fallible stream such that when | |||
There was a problem hiding this comment.
BTW I especially appreciate the understandable comments in this PR 🙏
Which issue does this PR close?
Extends on work done for #3462 - internal refactor
Rationale for this change
Internal refactor to allow sharing of the code between do_put and do_exchange methods in arrow-flight
What changes are included in this PR?
Refactored the poll_fn repetitions to use separate fallible streams for request and response streams.
Are there any user-facing changes?
No