Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(binding/c): Add blocking_reader for C binding #3259

Merged
merged 6 commits into from
Oct 14, 2023

Conversation

jiaoew1991
Copy link
Contributor

issue: #3257

@xyjixyjixyji
Copy link
Contributor

xyjixyjixyji commented Oct 12, 2023

To be more specific, we could have two functions to reach this approach

  1. opendal_reader_new() to construct a new Reader
  2. opendal_reader_buffer_read() to read through a Reader

For the first function, you could refer to the opendal_operator_new(). For the second part, your implementation in this PR will not change much.

WDYT?

@Xuanwo
Copy link
Member

Xuanwo commented Oct 12, 2023

2. opendal_reader_buffer_read() to read through a Reader

Negative. We should only have opendal_reader_read() that accepts an address passed by the C side.

@xyjixyjixyji
Copy link
Contributor

  1. opendal_reader_buffer_read() to read through a Reader

Negative. We should only have opendal_reader_read() that accepts an address passed by the C side.

Hmm, so we setup the reader (buffer size, path, etc.) on the first function, and then do the read that follows the configuration?

@Xuanwo
Copy link
Member

Xuanwo commented Oct 12, 2023

Hmm, so we setup the reader (buffer size, path, etc.) on the first function, and then do the read that follows the configuration?

The input write buffer size should be passed in every read call the same as in this PR.

I am concerned about the API semantics. We will not offer opendal_reader_buffer_read() because it implies that we will have a opendal_reader_read().

@jiaoew1991
Copy link
Contributor Author

Hi @Xuanwo @Ji-Xinyou , should I improve this pull request, or should I open a new one? 🤔

@Xuanwo
Copy link
Member

Xuanwo commented Oct 12, 2023

should I improve this pull request, or should I open a new one? 🤔

Both LGTM, I think you can work on this PR directly.

@Xuanwo Xuanwo changed the title Add blocking_read_with_buffer for C binding feat(binding/c): Add blocking_read_with_buffer for C binding Oct 13, 2023
@Xuanwo Xuanwo changed the title feat(binding/c): Add blocking_read_with_buffer for C binding feat(binding/c): Add blocking_reader for C binding Oct 13, 2023
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Others LGTM, cc @Ji-Xinyou would you like to take a review?

bindings/c/include/opendal.h Outdated Show resolved Hide resolved
@xyjixyjixyji
Copy link
Contributor

xyjixyjixyji commented Oct 13, 2023

All other parts LGTM, except for the return value of reader read. I mean it is always good to have as much error information as possible 🤣

@Xuanwo
Copy link
Member

Xuanwo commented Oct 13, 2023

Hi, @jiaoew1991, #3250 is about to merge. Maybe you can work upon it directly?

@jiaoew1991 jiaoew1991 force-pushed the blocking-read-buffer branch from 268dad2 to 4317cce Compare October 13, 2023 16:27
@jiaoew1991
Copy link
Contributor Author

@Xuanwo Updated it, but most of the CI jobs are cancelled 😅

@xyjixyjixyji
Copy link
Contributor

I closed and reopened this PR to rerun the CI.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 14, 2023

I found an issue that our naming for structs and functions is inconsistent. The word blocking shown up randomly.

cc @Ji-Xinyou, how about removing all blocking from our API for now? We can add async version in the future.

@xyjixyjixyji
Copy link
Contributor

I found an issue that our naming for structs and functions is inconsistent. The word blocking shown up randomly.

cc @Ji-Xinyou, how about removing all blocking from our API for now? We can add async version in the future.

Sure, that could be the default case since blocking is the most common.

When we add async in the future, we can add async keyword in the API.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 14, 2023

Sure, that could be the default case since blocking is the most common.

When we add async in the future, we can add async keyword in the API.

Cool, would you like to submit a PR after this one merged?

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks

@Xuanwo Xuanwo merged commit 8679cae into apache:main Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants