Use Slice<_> in write path instead of B: BoundedBuf<...> #8225
Conversation
3000 tests run: 2885 passed, 0 failed, 115 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
80f68a0 at 2024-07-02T13:49:38.717Z :recycle: |
pageserver/src/virtual_file.rs
Outdated
| /// Returns the IoBuf that is underlying the BoundedBuf `buf`. | ||
| /// I.e., the returned value's `bytes_init()` method returns something different than the `bytes_init()` that was passed in. | ||
| /// It's quite brittle and easy to mis-use, so, we return the size in the Ok() variant. | ||
| pub async fn write_all<B: BoundedBuf<Buf = Buf>, Buf: IoBuf + Send>( | ||
| pub async fn write_all<Buf: IoBuf + Send>( | ||
| &mut self, | ||
| buf: B, | ||
| buf: Slice<Buf>, | ||
| ctx: &RequestContext, | ||
| ) -> (B::Buf, Result<usize, Error>) { | ||
| ) -> (Slice<Buf>, Result<usize, Error>) { |
There was a problem hiding this comment.
- The doc comment is out of date.
- I think we should return the
Bufinstead of theSlice, so, the doc comments aren't out of date if you follow that suggestion.
Reason why we should return the Buf instead of the Slice<Buf>: the bounds aren't the same when we return, that could mis-lead users of this API.
If you want to keep the bounds, please take inspiration from the doc comment and assertions in read_exact_at.
There was a problem hiding this comment.
We had a 1:1 call and we agreed it is fine to have an API that mirrors the read API as long as the returned slice matches precisely the passed slice (like in the read API, see read_exact_at).
pageserver/src/virtual_file.rs
Outdated
| ctx: &RequestContext, | ||
| ) -> (B::Buf, Result<usize, Error>) { | ||
| ) -> (Slice<Buf>, Result<usize, Error>) { | ||
| let begin_end = (buf.begin(), buf.end()); |
| &mut self, | ||
| buf: B, | ||
| buf: Slice<Buf>, |
pageserver/src/virtual_file.rs
Outdated
| macro_rules! return_ { | ||
| ($buf:expr, $val:expr) => {{ | ||
| let buf = $buf.into_inner(); | ||
| return (buf.slice(begin_end.0..begin_end.1), $val); |
There was a problem hiding this comment.
After using buf.bounds() above, you can just pass buf.slice(begin_end) here.
| pub async fn write_blob<Buf: IoBuf + Send>( | ||
| &mut self, | ||
| srcbuf: B, | ||
| srcbuf: Slice<Buf>, | ||
| ctx: &RequestContext, | ||
| ) -> (B::Buf, Result<u64, Error>) { | ||
| ) -> (Slice<Buf>, Result<u64, Error>) { |
There was a problem hiding this comment.
No user of blob_io needs the Slice. Why change it?
There was a problem hiding this comment.
I guess there was a misunderstanding in our 1:1 call: I was strictly speaking about the VirtualFile API, you were probably thinking callers should be changed as well.
There was a problem hiding this comment.
Looking at the PR for why you're doing this one (#8106) I think we should leave write_blob's signature and blob_io.rs' write_all signature unchanged.
There was a problem hiding this comment.
I was strictly speaking about the VirtualFile API, you were probably thinking callers should be changed as well.
Ah yeah there was a misunderstanding. The reason why I made this PR was to change the API of BlobWriter::write_blob to workaround neondatabase/tokio-epoll-uring#46.
I didn't hit issues with VirtualFile, I only changed it because it was the path of least resistance to getting something that builds and where tests pass (and I thought you told me that that API design is better).
There was a problem hiding this comment.
No user of blob_io needs the Slice. Why change it?
to expand, there is no way to obtain B::Buf from B without slicing (please tell me if there is such a way). If B is an empty buffer, slicing panics. With a Slice, at least I can return srcbuf here.
There was a problem hiding this comment.
(please tell me if there is such a way)
let foo: B = ...;
let underlying: B::Buf = foo.slice_full().into_inner();
|
closing as Christian pointed out a way to merge #8106 without this PR ( |
Adopts
Slice<_>instead ofBoundedBuf<_>in the write path. This is a smaller version of what we could have done, as theOwnedAsyncWritertrait still uses the old API, but we leave this for the future. Mainly, this change is meant to unblock #8106.Prior PR: #8186 which did this for the read path.