Skip to content

Conversation

@kingsword09
Copy link
Contributor

Which issue does this PR close?

Related to #6281.

Rationale for this change

This PR adds support for opendal::options::ReadOptions and opendal::options::ReaderOptions conversion in the nodejs bindings. This is part of the migration to the new options API outlined in RFC-6213 (#6213).

What changes are included in this PR?

  • Added a complete mapping and conversion of opendal::options::ReadOptions and opendal::options::ReaderOptions
  • behavior tests mirroring Rust's async_read.rs test suite
  • Added the capabilities to support the options

Are there any user-facing changes?

Yes, users can now add options to their read and reader requests

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" labels Jun 18, 2025
@kingsword09 kingsword09 force-pushed the feat-nodejs-read-opts branch from 6ce3d5e to cb41253 Compare June 18, 2025 14:33
@kingsword09 kingsword09 force-pushed the feat-nodejs-read-opts branch from cb41253 to b456cac Compare June 18, 2025 14:42
describe.runIf(capability.read && capability.write)('async read options', () => {
test('read with range', async () => {
const size = 5 * 1024 * 1024
const size = 3 * 1024 * 1024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Limit range request to 3MB because the etcd service enforces a 4MB gRPC message size limit, and larger requests will fail.

https://github.com/apache/opendal/actions/runs/15731991430/job/44335059868?pr=6312#step:9:493

@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 18, 2025

@Xuanwo @suyanhanx We need to clarify whether the “size” parameter should represent the number of bytes to read after the offset (length-based), or if it should specify the absolute end bound (end-based) for the read operation.

I noticed that in the Python binding, “size” is actually used as the end bound, but the name “size” doesn’t seem to accurately reflect this usage.

with operator.open(filename, "rb", offset=range_start, size=range_end) as reader:

Currently, I am using “size” to indicate the number of bytes to read after the offset. Should I align this behavior with the Python binding, where “size” is used as the end bound?

@Xuanwo
Copy link
Member

Xuanwo commented Jun 19, 2025

I noticed that in the Python binding, “size” is actually used as the end bound, but the name “size” doesn’t seem to accurately reflect this usage.

Hi, I believe your understanding is correct. The size refers to this reading's size, and our actual read range should be offset..offset+size.

@Xuanwo
Copy link
Member

Xuanwo commented Jun 20, 2025

Hi, @kingsword09, is this PR ready to go?

@kingsword09
Copy link
Contributor Author

Hi, @kingsword09, is this PR ready to go?

Yes

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.

Nice work, let's go!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 20, 2025
@Xuanwo Xuanwo merged commit 062307d into apache:main Jun 20, 2025
254 checks passed
@kingsword09 kingsword09 deleted the feat-nodejs-read-opts branch June 20, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants