feat(object_store): Add put API for buffered::BufWriter
#5835
Conversation
Signed-off-by: Xuanwo <github@xuanwo.io>
|
I will try and review this PR later today or this weekend. Thanks @Xuanwo |
Thanks a lot! There is no hurry for this PR to get merged since we won't release soon. I just want to know about your plan first. |
|
Hi sorry, had a few too many things going on this week. I'll review this on Monday. This seems to have a lot in common with the existing BufWriter and so I wonder if we can combine them somehow |
There was a problem hiding this comment.
Thanks @Xuanwo -- I reviewed this PR and I think it looks very good to me. I have a few small comments on API design, naming and tests but I don't think they are critical
Even if some of this logic is duplicated (as @tustvold mentions in #5835 (comment)) I think as long as we have a good API and a good test, we can refactor to reduce the duplication as a follow on PR if we have time
Signed-off-by: Xuanwo <github@xuanwo.io>
|
Hi, @alamb, I have addressed all comments. PTAL, thanks! |
|
Hi apologies I have been ill the last few days, looking again at this I think we could simply achieve this by adding In my mind this would have a few key benefits:
What do you think? |
Wishing you a speedy recovery. Best wishes.
As long as we can avoid the extra |
Signed-off-by: Xuanwo <github@xuanwo.io>
|
Hi, @tustvold, I have added |
Xuanwo
changed the title
put API for buffered::BufWriter
Signed-off-by: Xuanwo <github@xuanwo.io>
| Ok(()) | ||
| } | ||
| BufWriterState::Write(None) | BufWriterState::Flush(_) => { | ||
| panic!("Already shut down") |
There was a problem hiding this comment.
Since this function returns a Result is there any reason to panic here rather than returning Err?
There was a problem hiding this comment.
Following the same behavior in poll_write. It should not happen that put come into this state.
I'm fine if we want to return error in both cases.
There was a problem hiding this comment.
Makes sense to me -- indeed poll_write has the same panic: https://github.com/apache/arrow-rs/pull/5835/files#diff-99b7e2564b0eef270679bfadfc81e23cfd1565dc7e8e6a029192856439da6c7eL322
| self.state = BufWriterState::Write(f.await?.into()); | ||
| continue; | ||
| } | ||
| BufWriterState::Buffer(path, b) => { |
There was a problem hiding this comment.
i took a look at trying to combine this code with the very similar code in poll_write but I couldn't come up with anything that was particularly good
There was a problem hiding this comment.
Yes, it's a bit clumsy to refactor the same code for both poll-based and async-await systems just for 10 lines.
There was a problem hiding this comment.
The problem here is I want to avoid the extra cost of Box::pin(fut) by awaiting it in place.
|
Thanks again @Xuanwo |
Which issue does this PR close?
Closes #5834
Rationale for this change
Addbuffered::BufUploaderto provide buffered support via PayLoad native API.Add
putAPI forbuffered::BufWriterWhat changes are included in this PR?
New struct:buffered::BufUploaderbuffered::BufWriternow has new async APIput(Bytes).Are there any user-facing changes?
New API:buffered::BufUploaderNew API:
buffered::BufWriter::put