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(iroh): add more rpc methods #1962

Merged
merged 5 commits into from
Jan 23, 2024
Merged

feat(iroh): add more rpc methods #1962

merged 5 commits into from
Jan 23, 2024

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Jan 19, 2024

  • add blobs.read_at: Allows to efficiently read parts of a blob via RPC
  • add blobs.get_collection: read a collection

Allows to efficiently read parts of a blob via RPC
@Frando
Copy link
Member

Frando commented Jan 19, 2024

Cool I thought to add this quite a while ago already. Is there a reason to keep both BlobRead and BlobReadAt rpcs? Might be simpler to just have a single BlobRead RPC with offset: Option<usize>, limit: Option<usize>?

DownloadProgress, ListTagsRequest, ListTagsResponse, NodeConnectionInfoRequest,
NodeConnectionInfoResponse, NodeConnectionsRequest, NodeShutdownRequest, NodeStatsRequest,
NodeStatusRequest, NodeStatusResponse, ProviderService, SetTagOption, ShareMode, WrapOption,
BlobListRequest, BlobListResponse, BlobReadAtRequest, BlobReadAtResponse, BlobReadRequest,
Copy link
Contributor

Choose a reason for hiding this comment

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

This import section seems to indicate that wildcard imports aren't that bad after all....

iroh/src/node.rs Outdated
(1, len)
} else {
(
(len as f64 / max_chunk_size as f64).ceil() as usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

weird to use f64 here... It is shorter than the integer variant, but will fail if len or max_chunk_size is > 2^53.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would I round up otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

len / max_chunk_size + if len % max_chunk_size == 0 { 0 } else { 1 }

or

len / max_chunk_size + (len % max_chunk_size !=0) as usize

or something. With a bit of luck the optimizer will do only 1 integer division for both division and rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I don't worry about speed here. It's just weird to use fp for integer ops, and it will fail in weird ways as soon as you exceed the range of integers that can be losslessly represented in a fp num.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, that is of course nice

iroh/src/node.rs Show resolved Hide resolved
allows fetching a collection via rpc
@dignifiedquire dignifiedquire changed the title feat(iroh): add blob.read_at method feat(iroh): add more rpc methods Jan 19, 2024
@dignifiedquire
Copy link
Contributor Author

Might be simpler to just have a single BlobRead RPC wit

done

Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

avoid the floats!

@dignifiedquire dignifiedquire added this pull request to the merge queue Jan 23, 2024
Merged via the queue into main with commit 4910df1 Jan 23, 2024
18 checks passed
@dignifiedquire dignifiedquire deleted the feat-blob-read-at branch January 23, 2024 10:57
fubuloubu pushed a commit to ApeWorX/iroh that referenced this pull request Feb 21, 2024
- add `blobs.read_at`: Allows to efficiently read parts of a blob via
RPC
- add `blobs.get_collection`: read a collection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants