You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "WenyXu (via GitHub)" <gi...@apache.org> on 2023/12/10 08:32:56 UTC
[PR] RFC: Buffered reader [incubator-opendal]
WenyXu opened a new pull request, #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734
This RFC proposes a buffered reader to amortize the overhead of IO. The concurrent fetching feature will be introduced in other RFCs soon.
## Related
- #3725
- #3726
- https://github.com/GreptimeTeam/greptimedb/issues/2891
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424346024
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
> `ReadExtra` or just `Read` is fine I think.
>
> It's just that the combination of `ReadToEnd` + `limit` doesn't quite fit with my experience.
The `Read` looks good to me😙, I will change it later today
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
> `ReadExtra` or just `Read` is fine I think.
>
> It's just that the combination of `ReadToEnd` + `limit` doesn't quite fit with my experience.
The `Read` looks good to me😙, I will change it later today
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425121708
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
OK, I know there is `seek_relative`, but I think many format readers invoke the `seek` and `read` methods, resulting in inefficient.
https://doc.rust-lang.org/std/io/struct.BufReader.html#method.seek
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424954446
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
@Xuanwo, I've checked the behaviors of `std::io::BufReader` and `tokio::io::BufferReader` in relation to our RFC. The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method. My colleague @zhongzc also mentioned that its behavior should be more akin to the `Cache`(or `PageCache`) than the `Buffer`. And it might be a little misleading if we also called our implementation `BufReader`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425073323
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> And it might be a little misleading if we also called our implementation `BufReader`.
I'm OK with either names. They are not part of public API so users won't operate on buffer reader directly.
We can discuss the naming on PR instead, not a blocker of RFC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424024163
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
> Could you elaborate on how a buffer will be beneficial in this scenario?
>
> ```rust
> let r = op.reader(path).await;
> r.seek(SeekFrom::End(-8)).await;
> r.read_to_end(&Vec::new()).await;
> ```
>
> Seems buffer is not helpful.
Oh, I misunderstood before; in this scenario, invoking `r.read_to_end(& Vec: :new ()).await`; will execute a read of the file within the range (-8, end).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423954377
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
> scenario
Yes, In this scenario, the buffer isn’t helpful. I think we might need something
called tailing buffer.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
> scenario
Yes, In this scenario, the buffer isn’t helpful. I think we might need something
called tailing buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424056307
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
Review Comment:
Or we can add a fast path for default behavior.
i.e.,
```rust
if args. buffer.is_none() {
self.complete_read(path, args).await
} else {
...
}
```
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
Review Comment:
Or we can add a fast path for default behavior.
i.e.,
```rust
if args.buffer.is_none() {
self.complete_read(path, args).await
} else {
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425334003
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
Yes. Let me convert the RFC to a draft and think about how to implement the feature we talked about above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1421707610
##########
core/src/docs/rfcs/buffered_reader.md:
##########
@@ -0,0 +1,112 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: TBD
+- Tracking Issue: TBD
Review Comment:
I'm going to add these fields later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1848896038
I'm going to invite my workmate to review this RFC, too. cc @evenyag @zhongzc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1422010283
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+There is an invariant: the `Buffer Capability` is at least greater than or equal to the `Preferred IO Size`.
Review Comment:
Let's focus on how buffer works here. I believe it's different with `preferred io size`.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
Review Comment:
`read_size` seems not a good API and is not related to `buffer` API, how about remove from this RFC?
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
Review Comment:
The default io size is different for different services, let's take `fs` and `s3` as examples:
- `fs`: opendal forward the `buf` directly, so the io size is controlled by users input.
- `s3`: opendal can't control the underlying read io size (which is usually controlled by system's TCP stack). OpenDAL sends requests with the whole range.
So there are two problems:
- For `fs`, the io size could be too small. For example, users reading data with a `&mut [u8:1]`
- For `s3`, the io size could be too large. For example, users only want to read 1MiB, but we send a request for the whole file size.
`buffer` can give users a way to control the underlying read behavior. Besides, buffer allows user to seek inside already fetched data without sending new requests.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
Review Comment:
I believe they should be separate entities and we shouldn't include them in the same RFCs.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+There is an invariant: the `Buffer Capability` is at least greater than or equal to the `Preferred IO Size`.
+
+**Buffering**
+
+The target file will be split into multiple segments to align with the `Preferred IO Size`, and the buffered segments also align with the `Preferred IO Size` for efficient lookup.
Review Comment:
Will this RFC cover `multiple segments`? I feel like they should be another topic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424024163
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
> Could you elaborate on how a buffer will be beneficial in this scenario?
>
> ```rust
> let r = op.reader(path).await;
> r.seek(SeekFrom::End(-8)).await;
> r.read_to_end(&Vec::new()).await;
> ```
>
> Seems buffer is not helpful.
Oh, I misunderstood before; in this scenario, invoking `r.read_to_end(&Vec::new()).await`; will execute a read of the file within the range (-8, end).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "zhongzc (via GitHub)" <gi...@apache.org>.
zhongzc commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424299124
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
`limit` appears here for the first time, and I still have difficulty understanding it. For `read_to_end`, it means reading to the end of the file. Do you mean `read_extra`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424884361
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
Review Comment:
> Do we really need a range? As mentioned in previous discussion, we will focuse on the most simple buf read case, and leave tailing read outside. So we don't need to track the range of this buffer. It will always be the content in `offset..offset + buf.len()`.
I agree, an offset will be enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1429994775
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,64 @@
+- Proposal Name: `buffered_reader`
Review Comment:
Please also add this file in `rfcs/mod.rs` so it can be generated in our docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC-3734: Buffered reader [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1860927069
For me, mostly LGTM. BTW, I have a suggestion here, maybe we need to add a `Capability` to let each service's developer decide support the buffer read or not and check the maximum size their own.
Because some of the service like PGSQL,MySQL etc.. are dedicated to the small file.
This is probably an implementation issue, I'm not sure we need add extra description in RFC docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC-3734: Buffered reader [incubator-opendal]
Posted by "hoosin (via GitHub)" <gi...@apache.org>.
hoosin commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1862040065
cool
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "zhongzc (via GitHub)" <gi...@apache.org>.
zhongzc commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424323778
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
Consider the following scenario:
The last four bytes represent the size of the metadata, and the code for reading the metadata might look like:
```rust
reader.seek(SeekFrom::End(-4));
let mut buf = [0u8; 4];
reader.read_exact(&mut buf)?;
let size = u32::from_le_bytes(buf);
reader.seek(SeekFrom::End(-(4 + size as i64)))?;
let mut buf = vec![0u8; size as usize];
reader.read_exact(&mut buf)?;
let metadata = parse_metadata(&buf)?;
```
According to the design presented here, would this lead to two separate calls to the underlying Seek and Read operations, rendering the use of a buffer ineffective?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424874361
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
Review Comment:
Can we use `RangeBound<usize>` as the buffered range?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425121708
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
OK, I know there is `seek_relative`, but I think many readers invoke the `seek` and `read` methods, resulting in ineffective. https://doc.rust-lang.org/std/io/trait.Seek.html#tymethod.seek
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425150436
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> OK, I know there is `seek_relative`, but I think many format readers just naively invoke the `seek` and `read` methods, resulting in inefficient.
Got it. OpenDAL's Reader doesn't provide `into_inner` like `BufReader`, so it's safe for us to always use seek_relative while ever possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "zhongzc (via GitHub)" <gi...@apache.org>.
zhongzc commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1422055758
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
Review Comment:
I don't quite understand what this legend is trying to convey 🥲
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
Review Comment:
I don't quite understand what this legend is trying to convey 🥲
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424882929
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
Review Comment:
Got it👍
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
Review Comment:
Got it👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424363783
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
Yes, it is. Now the `read` of `RangeReader` is super expensive.
> RangeReader will send request to read the entire range everytime no matter how large the read buffer size is.
See also #3747
However, the `BufferReader` might not work well If the `read` always bypasses the buffer🥹.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
Yes, it is. Now the `read` of `RangeReader` is super expensive.
> RangeReader will send request to read the entire range everytime no matter how large the read buffer size is.
See also #3747
However, the `BufferReader` might not work well If the `read` always bypasses the buffer🥹.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425121708
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
OK, I know there is `seek_relative`, but I think many format readers invoke the `seek` and `read` methods, resulting in ineffective. https://doc.rust-lang.org/std/io/trait.Seek.html#tymethod.seek
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423676321
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
Review Comment:
I removed the redundant concepts and added some figures, which should be easier to understand than before.🥹
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
Review Comment:
I removed the redundant concepts and added some figures, which should be easier to understand than before.🥹
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423954377
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
> scenario
Yes, In this scenario, the buffer isn’t helpful. I think we might need something called the tailing buffer.
Let's split these into two types of buffers because if a reader is required to read the tailing of the file, it is generally to read some very small metadata; see also #3745 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC-3734: Buffered reader [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1860964418
> Users don't care about buffer can just don't use this API.
>
> And besides, buffer is useful even when file is small. For example, read(1), seek(1), read(2).
Make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1421711547
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
Review Comment:
Maybe let the `Accessor`'s metadata provide the default value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1421906959
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
Review Comment:
Users might specify a buffer size that is less than the read size or isn't aligned with the read size.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
Review Comment:
Consider providing a link to the ParquetStream.
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+There is an invariant: the `Buffer Capability` is at least greater than or equal to the `Preferred IO Size`.
+
+**Buffering**
+
+The target file will be split into multiple segments to align with the `Preferred IO Size`, and the buffered segments also align with the `Preferred IO Size` for efficient lookup.
Review Comment:
Could you further explain how the reader buffers multiple segments and when to fetch the next segment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "zhongzc (via GitHub)" <gi...@apache.org>.
zhongzc commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424346366
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
Just to confirm, does the goal of this RFC include reducing the number of `seek` and `read` calls on the underlying storage?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424773796
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> How about implementing a basic buffer similar to `std::io::BufReader` and postponing the `tail read` feature? It seems that specific file formats are too complex for a reader (AsyncRead & AsyncSeek) to handle effectively.
Yes, the tailing buffer feature isn't part of this RFC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423943001
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
Review Comment:
*Reference-level explanation* should go deep into the code to explain how will you implement this feature in general. For example:
- Will you need to add a new struct?
- Will you finish those work at CompleteLayer?
- How will you make it works for all services?
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
Could you elaborate on how a buffer will be beneficial in this scenario?
```rust
let r = op.reader(path).await;
r.seek(SeekFrom::End(-8)).await;
r.read_to_end(&Vec::new()).await;
```
Seems buffer is not helpful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424880733
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
Review Comment:
Do we really need a range? As mentioned in previous discussion, we will focuse on the most simple buf read case, and leave tailing read outside. So we don't need to track the range of this buffer. It will always be the content in `offset..offset + buf.len()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424879003
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
Review Comment:
Yes, Let me change it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424954446
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
@Xuanwo, I've checked the behaviors of `std::io::BufReader` and `tokio::io::BufferReader` in relation to our RFC. The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method. My colleague @zhongzc also mentioned that its behavior should be more akin to the `Cache` than the `Buffer`. And it might be a little misleading if we also called our implementation `BufReader`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425121708
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
OK, I know there is `seek_relative`, but I think many format readers invoke the `seek` and `read` methods, resulting in inefficient. https://doc.rust-lang.org/std/io/trait.Seek.html#tymethod.seek
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC-3734: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1860955816
> BTW, I have a suggestion here, maybe we need to add a Capability to let each service's developer decide support the buffer read or not and check the maximum size their own.
Users don't care about buffer can just don't use this API.
And besides, buffer is useful even when file is small. For example, read(1), seek(1), read(2).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423677103
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+There is an invariant: the `Buffer Capability` is at least greater than or equal to the `Preferred IO Size`.
+
+**Buffering**
+
+The target file will be split into multiple segments to align with the `Preferred IO Size`, and the buffered segments also align with the `Preferred IO Size` for efficient lookup.
Review Comment:
Yes, I removed this redundant concept.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423680041
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,113 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
Review Comment:
I updated the latest version with a text explanation instead of this example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424308704
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
Oh, I got you; it works like `Take(limit).ReadToEnd(),` actually... I will rename it to a better one. Would you happen to have any idea?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1423954377
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,150 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+```
+File
Review Comment:
> Could you elaborate on how a buffer will be beneficial in this scenario?
>
> ```rust
> let r = op.reader(path).await;
> r.seek(SeekFrom::End(-8)).await;
> r.read_to_end(&Vec::new()).await;
> ```
>
> Seems buffer is not helpful.
Yes, In this scenario, the buffer isn’t helpful. I think we might need something called the tailing buffer.
Let's split these into two types of buffers because if a reader is required to read the tailing of the file, it is generally to read some very small metadata; see also #3745 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424787945
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
SO we don't need to discuss different cases one by one, right? How about refer to `std::io::BufReader` directly?
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
So we don't need to discuss different cases one by one, right? How about refer to `std::io::BufReader` directly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1421708313
##########
core/src/docs/rfcs/buffered_reader.md:
##########
@@ -0,0 +1,112 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: TBD
+- Tracking Issue: TBD
+
+# Summary
+
+Amortize overhead of IO.
+
+# Motivation
+
+The aim is to amortize the overhead of IO. Some scenarios, such as importing data to the database, typically require supporting multiple file formats, and the size of files varies(very large or very small). Most readers of these formats may behave similarly to the `ParquetStream`; they read and decode data piece after piece.
+
+```
+File
+┌───┬───┬───┬──┐
+│ │ │ │ │
+└───┴───┴───┘▲─┘
+ │
+ │ 1. SeekFromEnd(8)
+
+ 2. ReadToEnd(limit)
+```
+
+However, OpenDAL will yield an IO request after `seek()` and `read()` are invoked. For storage services such as S3, The [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) shows that the 8MiB~16MiB is the preferred IO size, If the IO size is too small, the TTFB dominates the overall runtime; too large will reach the bandwidth limit.
+
+Therefore, This RFC proposes a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+If a user prefers to customize the `Preferred IO Size`, it provides a `read_size` option (default is 8MiB):
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).read_size(8 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+There is an invariant: the `Buffer Capability` is at least greater than or equal to the `Preferred IO Size`.
Review Comment:
If the file size is smaller than the `Preferred IO Size`, the file will be buffered fully after the `read()` is invoked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1849221681
Another idea came to my mind: we can support multiple buffering policies for different purposes. i.e., only buffer the partial and the prefetched segments (which will be introduced with the concurrent fetching feature) for future use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424954446
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
@Xuanwo, I've checked the behaviors of `std::io::BufReader` and `tokio::io::BufferReader` in relation to our RFC. The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method. My colleague @zhongzc also mentioned that its behavior should be more akin to the `Cache` than the `Buffer`. And it might be a little misleading if we also called our implementation `BufReader`.
The first version, `ChunkCacheReader,` might sound good to us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425106501
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
So, back to the earlier suggestion, I think we can't simply refer to `std::io::BufReader`. It may be better to discuss different cases one by one. Would you happen to have any suggestions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425121708
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
OK, I know there is `seek_relative`, but I think many format readers just naively invoke the `seek` and `read` methods, resulting in inefficient.
https://doc.rust-lang.org/std/io/struct.BufReader.html#method.seek
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425073323
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> And it might be a little misleading if we also called our implementation `BufReader`.
I'm OK with either names. They are not part of public API so users won't operate on buffer reader directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "zhongzc (via GitHub)" <gi...@apache.org>.
zhongzc commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424333119
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
`ReadExtra` or just `Read` is fine I think.
It's just that the combination of `ReadToEnd` + `limit` doesn't quite fit with my experience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424793106
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> So we don't need to discuss different cases one by one, right? How about refer to `std::io::BufReader` directly?
OK, I will check the `std::io::BufReader` docs and implementation later to see if I need to add supplementary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424747256
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> Now the `read` of `RangeReader` is super expensive.
Correct: Both `read` and `seek` is not expensive, only the case that `read` after `seek` expensive. As explained in https://github.com/apache/incubator-opendal/issues/3747, we predict users to read the whole range, but they don't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424954446
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
@Xuanwo, I've checked the behaviors of `std::io::BufReader` and `tokio::io::BufferReader` to our RFC. The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method. My colleague @zhongzc also mentioned that its behavior should be more akin to the `Cache` than the `Buffer`. And it might be a little misleading if we also called our implementation `BufReader`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "oowl (via GitHub)" <gi...@apache.org>.
oowl commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425420619
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
+
+```
+File
+┌──────────────┐
+│ │
+└────────────▲─▲
+ │ │
+ │ │ 2. ReadToEnd(limit)
+ │
+ │ 1. SeekFromEnd(-8)
+```
+
+**Overlapping Read**
+
+If a read request attempts to read data that is partially buffered, it copies the partially buffered data to the `dst` buffer first.
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───────┬──────┐
+│*******│ │
+└────▲──┴──▲───┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+```
+
+Then, fetches and buffers data in chunks corresponding to the buffer size. Finally, it copies the another part data to the `dst` buffer.
+
+```
+Buffer
+┌───────┐
+│///////│
+└───────┘
+File
+┌───────┬──────┐
+│ │//////│
+└────▲──┴──▲───┘
+ 3. Fetches and buffers the data.
+```
+
+**Bypass**
+
+If a read request attempts to read data larger than the `Buffer Capability`, it fetches the data bypassing the `Buffer` entirely.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───────────┬──┐
+│ │**│
Review Comment:
Can you explain what the blacked-out part in the back of this confusing picture is? It seems that doesn't have any meaning in this scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1860197549
Sorry for the delay. Please take a look when you have time. Thanks🥹
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC-3734: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#issuecomment-1860969148
We got three approvals now, merging!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC-3734: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425432935
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
+
+```
+File
+┌──────────────┐
+│ │
+└────────────▲─▲
+ │ │
+ │ │ 2. ReadToEnd(limit)
+ │
+ │ 1. SeekFromEnd(-8)
+```
+
+**Overlapping Read**
+
+If a read request attempts to read data that is partially buffered, it copies the partially buffered data to the `dst` buffer first.
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───────┬──────┐
+│*******│ │
+└────▲──┴──▲───┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+```
+
+Then, fetches and buffers data in chunks corresponding to the buffer size. Finally, it copies the another part data to the `dst` buffer.
+
+```
+Buffer
+┌───────┐
+│///////│
+└───────┘
+File
+┌───────┬──────┐
+│ │//////│
+└────▲──┴──▲───┘
+ 3. Fetches and buffers the data.
+```
+
+**Bypass**
+
+If a read request attempts to read data larger than the `Buffer Capability`, it fetches the data bypassing the `Buffer` entirely.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───────────┬──┐
+│ │**│
Review Comment:
Yes, I will remove it later. It's kind of confusing. Thank🥰
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
+
+```
+File
+┌──────────────┐
+│ │
+└────────────▲─▲
+ │ │
+ │ │ 2. ReadToEnd(limit)
+ │
+ │ 1. SeekFromEnd(-8)
+```
+
+**Overlapping Read**
+
+If a read request attempts to read data that is partially buffered, it copies the partially buffered data to the `dst` buffer first.
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───────┬──────┐
+│*******│ │
+└────▲──┴──▲───┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+```
+
+Then, fetches and buffers data in chunks corresponding to the buffer size. Finally, it copies the another part data to the `dst` buffer.
+
+```
+Buffer
+┌───────┐
+│///////│
+└───────┘
+File
+┌───────┬──────┐
+│ │//////│
+└────▲──┴──▲───┘
+ 3. Fetches and buffers the data.
+```
+
+**Bypass**
+
+If a read request attempts to read data larger than the `Buffer Capability`, it fetches the data bypassing the `Buffer` entirely.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───────────┬──┐
+│ │**│
Review Comment:
Yes, I will remove it later. It's kind of confusing. Thank🥰
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "zhongzc (via GitHub)" <gi...@apache.org>.
zhongzc commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424337514
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
Or perhaps there's no need to change it; it might just be that I lack relevant experience. It seems take(limit).read_to_end(buf) is a common practice (I'm just not well-informed on the matter).
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
Or perhaps there's no need to change it; it might just be that I lack relevant experience. It seems `take(limit).read_to_end(buf)` is a common practice (I'm just not well-informed on the matter).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424336275
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
Yes, it will. I'm also considering introducing the tailing buffer feature #3745. Theoretically, this feature could reduce latency by around 25ms when reading trailing bytes from S3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424338361
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
How about just `Read(num_bytes)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424308704
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
Oh, I got you; it works like `Take(limit).ReadToEnd()`, actually... I will rename it to a better one. Would you happen to have any idea?
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
Review Comment:
Oh, I got you; it works like `Take(limit).ReadToEnd()` actually... I will rename it to a better one. Would you happen to have any idea?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424954446
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
@Xuanwo, I've checked the behaviors of `std::io::BufReader` and `tokio::io::BufferReader` in relation to our RFC. The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method. My colleague @zhongzc also mentioned that its behavior should be more akin to the `Cache` than the `Buffer`. And it might be a little misleading if we also called our implementation `BufReader`.
The first version of the RFC, which maybe should be called `ChunkCacheReader`, might sound good to us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424750501
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
How about implementing a basic buffer similar to `std::io::BufReader` and postponing the `tail read` feature? It seems that specific file formats are too complex for a reader (AsyncRead & AsyncSeek) to handle effectively.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424954446
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
@Xuanwo, I've checked the behaviors of `std::io::BufReader` and `tokio::io::BufferReader` in relation to our RFC. The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method. My colleague @zhongzc also mentioned that its behavior should be more akin to the `Cache` than the `Buffer`. And it might be a little misleading if we also called our implementation `BufReader`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1425113212
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
+
+A `buffer` field of type `Option<usize>` will be introduced to `OpRead`. If `buffer` is set to `None`, it functions with default behavior. However, if buffer is set to `Some(n)`, it denotes the maximum buffer capability that the `BufferReader` can utilize, and buffering behaviors introduced below.
+
+**Buffering**
+
+When the `Buffer Capability` is specified, the underlying reader fetches and buffers data in chunks corresponding to the buffer size.
+
+```
+Buffer
+┌───────┐
+│ │
+└───────┘
+File
+┌───┬───────┬──┐
+│ │ │ │
+└───▲───────▲──┘
+ │ │
+ │ │ 2. ReadToEnd(limit), The limit <= Cap(Buffer).
+ │
+ │ 1. SeekFromStart(128)
+
+```
+
+```
+Buffer
+┌───────┐
+│*******│
+└───────┘
+File
+┌───┬───────┬──┐
+│ │*******│ │
+└───▲───────▲──┘
+ 3. Fetches and buffers the data.
+```
+
+**Tailing Read**
+
+If a read request attempts to read the trailing bytes of the file, it only reads and buffers the `Min(Max(Limit,Cap(Buffer)),FileEnd-Cursor)` bytes.
Review Comment:
> The key distinction is that both `BufReader` implementations consistently discard the inner buffer when invoking the `seek` method.
This understanding is incorrect: https://doc.rust-lang.org/std/io/struct.BufReader.html#method.seek_relative
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RFC: Buffered reader [incubator-opendal]
Posted by "WenyXu (via GitHub)" <gi...@apache.org>.
WenyXu commented on code in PR #3734:
URL: https://github.com/apache/incubator-opendal/pull/3734#discussion_r1424056307
##########
core/src/docs/rfcs/3734_buffered_reader.md:
##########
@@ -0,0 +1,175 @@
+- Proposal Name: `buffered_reader`
+- Start Date: 2023-12-10
+- RFC PR: [apache/incubator-opendal#3574](https://github.com/apache/incubator-opendal/pull/3734)
+- Tracking Issue: [apache/incubator-opendal#3575](https://github.com/apache/incubator-opendal/issues/3735)
+
+# Summary
+
+Allowing the underlying reader to fetch data at the buffer's size to amortize the IO's overhead.
+
+# Motivation
+
+The objective is to mitigate the IO overhead. In certain scenarios, the reader processes the data incrementally, meaning that it utilizes the `seek()` function to navigate to a specific position within the file. Subsequently, it invokes the `read()` to reads `limit` bytes into memory and performs the decoding process.
+
+
+OpenDAL triggers an IO request upon invoking `read()` if the `seek()` has reset the inner state. For storage services like S3, [research](https://www.vldb.org/pvldb/vol16/p2769-durner.pdf) suggests that an optimal IO size falls within the range of 8MiB to 16MiB. If the IO size is too small, the Time To First Byte (TTFB) dominates the overall runtime, resulting in inefficiency.
+
+Therefore, this RFC proposes the implementation of a buffered reader to amortize the overhead of IO.
+
+# Guide-level explanation
+
+For users who want to buffered reader, they will call the new API `buffer`. And the default behavior remains unchanged, so users using `op.reader_with()` are not affected. The `buffer` function will take a number as input, and this number will represent the maximum buffer capability the reader is able to use.
+
+```rust
+op.reader_with(path).buffer(32 * 1024 * 1024).await
+```
+
+# Reference-level explanation
+
+This feature will be implemented in the `CompleteLayer`, with the addition of a `BufferReader` struct in `raw/oio/reader/buffer_reader.rs`.
+
+The `BufferReader` employs` bytes::BytesMut` as the inner buffer and utilizes `(Option<usize>, Option<usize>)` to track the buffered range of the file.
+
+
+```rust
+ ...
+ async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> {
+ BufferReader::new(self.complete_read(path, args).await)
+ }
+
+ ...
+
+ fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> {
+ BufferReader::new(self.complete_blocking_read(path, args))
+ }
+ ...
+```
Review Comment:
Or we can add a fast way for default behavior.
i.e.,
```rust
if args. buffer.is_none() {
self.complete_read(path, args).await
} else {
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org