Skip to content

RFC-3734: Buffered reader #3734

Merged
Xuanwo merged 5 commits into
apache:mainfrom
WenyXu:docs/buffer-reader-rfc
Dec 18, 2023
Merged

RFC-3734: Buffered reader #3734
Xuanwo merged 5 commits into
apache:mainfrom
WenyXu:docs/buffer-reader-rfc

Conversation

@WenyXu

@WenyXu WenyXu commented Dec 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

This RFC proposes a buffered reader to amortize the overhead of IO. The concurrent fetching feature will be introduced in another RFC soon.

Related

@WenyXu
WenyXu requested a review from Xuanwo as a code owner December 10, 2023 08:32
WenyXu
WenyXu commented Dec 10, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/buffered_reader.md Outdated
@WenyXu

WenyXu commented Dec 10, 2023

Copy link
Copy Markdown
Member Author

I'm going to invite my workmate to review this RFC, too. cc @evenyag @zhongzc

WenyXu
WenyXu commented Dec 10, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/buffered_reader.md Outdated
@WenyXu
WenyXu force-pushed the docs/buffer-reader-rfc branch from bb87a31 to 268b142 Compare December 10, 2023 08:44
@WenyXu WenyXu mentioned this pull request Dec 10, 2023
Closed
11 tasks
@WenyXu
WenyXu force-pushed the docs/buffer-reader-rfc branch from 268b142 to 83f10e8 Compare December 10, 2023 08:57
WenyXu
WenyXu commented Dec 10, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
@WenyXu

WenyXu commented Dec 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Another idea came to my mind: we can support multiple buffering policies for different purposes. i.e., only buffer the partial read and prefetched segments for future use(the segment prefetching will be introduced with the concurrent fetching feature). Maybe we should leave it as future work.

evenyag
evenyag reviewed Dec 11, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Xuanwo
Xuanwo reviewed Dec 11, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
zhongzc
zhongzc reviewed Dec 11, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
@WenyXu
WenyXu marked this pull request as draft December 11, 2023 10:13
@Xuanwo Xuanwo added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Dec 11, 2023
@WenyXu
WenyXu marked this pull request as ready for review December 12, 2023 09:05
@WenyXu WenyXu mentioned this pull request Dec 12, 2023
Closed
Xuanwo
Xuanwo reviewed Dec 12, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
@WenyXu
WenyXu force-pushed the docs/buffer-reader-rfc branch from 1e4015f to 2dbced7 Compare December 12, 2023 14:07
WenyXu
WenyXu commented Dec 12, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md
@WenyXu
WenyXu force-pushed the docs/buffer-reader-rfc branch from 2dbced7 to 13d7cdf Compare December 12, 2023 16:31
zhongzc
zhongzc reviewed Dec 12, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
zhongzc
zhongzc reviewed Dec 12, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
xyjixyjixyji
xyjixyjixyji reviewed Dec 13, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
@WenyXu
WenyXu marked this pull request as draft December 13, 2023 13:10
oowl
oowl reviewed Dec 13, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md Outdated
@WenyXu
WenyXu force-pushed the docs/buffer-reader-rfc branch from bf07ef1 to 6400337 Compare December 18, 2023 11:10
@WenyXu
WenyXu force-pushed the docs/buffer-reader-rfc branch from 6400337 to fa960e5 Compare December 18, 2023 11:15
@WenyXu
WenyXu marked this pull request as ready for review December 18, 2023 11:15
@WenyXu

WenyXu commented Dec 18, 2023

Copy link
Copy Markdown
Member Author

Sorry for the delay. Please take a look when you have time. Thanks🥹

Xuanwo
Xuanwo approved these changes Dec 18, 2023
View reviewed changes

@Xuanwo Xuanwo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Mostly LGTM.

Xuanwo
Xuanwo reviewed Dec 18, 2023
View reviewed changes
Comment thread core/src/docs/rfcs/3734_buffered_reader.md
@Xuanwo Xuanwo changed the title RFC: Buffered reader RFC-3734: Buffered reader Dec 18, 2023
@Zheaoli

Zheaoli commented Dec 18, 2023

Copy link
Copy Markdown
Member

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

@Xuanwo

Xuanwo commented Dec 18, 2023

Copy link
Copy Markdown
Member

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).

@Zheaoli

Zheaoli commented Dec 18, 2023

Copy link
Copy Markdown
Member

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.

Zheaoli
Zheaoli approved these changes Dec 18, 2023
View reviewed changes

@Zheaoli Zheaoli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Xuanwo

Xuanwo commented Dec 18, 2023

Copy link
Copy Markdown
Member

We got three approvals now, merging!

@Xuanwo
Xuanwo merged commit b0f759d into apache:main Dec 18, 2023
@hoosin

hoosin commented Dec 19, 2023

Copy link
Copy Markdown

cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes
@xyjixyjixyji xyjixyjixyji xyjixyjixyji left review comments
@Zheaoli Zheaoli Zheaoli approved these changes
@oowl oowl oowl approved these changes
+2 more reviewers
@evenyag evenyag evenyag left review comments
@zhongzc zhongzc zhongzc left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat"

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@WenyXu @Zheaoli @Xuanwo @hoosin @evenyag @zhongzc @oowl @xyjixyjixyji