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: range request support #330

Merged
merged 5 commits into from
Oct 14, 2022
Merged

feat: range request support #330

merged 5 commits into from
Oct 14, 2022

Conversation

Arqu
Copy link
Collaborator

@Arqu Arqu commented Oct 12, 2022

@Arqu Arqu added the c-gateway label Oct 12, 2022
@Arqu Arqu added this to the v0.1.0 milestone Oct 12, 2022
@Arqu Arqu self-assigned this Oct 12, 2022
}

fn poll_complete(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<std::io::Result<u64>> {
Poll::Ready(Ok(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong return value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above, would love some input on what it should be. I'm assuming the delta from the initial position which in a simplified version returns the seek offset?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think this is the delta, so we need to store it and return it. Later when we do seeking with loading, this will also delegate to an internal future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I took some time to understand this and as far as I get it it's not the delta but rather the final position it seeked to. Because otherwise the consumer would need to figure out what the delta means especially in circular buffer and negative moves etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes new offset from the start is what should be returned 👍

Base automatically changed from arqu/gw_spec_parity to main October 14, 2022 08:11
@Arqu
Copy link
Collaborator Author

Arqu commented Oct 14, 2022

@dignifiedquire can you take another look at the unixfs seeking, it's funkier now.
Also added tests and cleaned it up.

}
}

#[tokio::test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Either in this PR or at least have an issue for seeking across dag-cbor, dag-json and raw files as well please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotta do other stuff before release :/

@dignifiedquire
Copy link
Contributor

merge conflict 😅

dignifiedquire
dignifiedquire previously approved these changes Oct 14, 2022
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

Looks good, please also open an issue to add some additional tests on the seeking math, that is easy to get wrong on edge cases and I don't trust my own review of them. Some nice proptests should help for confidence.

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.

feat(gateway): add support for range requests
2 participants