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

Deliver fetch data on fetch stream #527

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Deliver fetch data on fetch stream #527

wants to merge 13 commits into from

Conversation

RichLogan
Copy link
Contributor

@RichLogan RichLogan commented Mar 14, 2025

I think this sends fetch data on a single stream with FETCH_HEADER followed by N FETCH_OBJECTs.

The easiest way to do this seemed to be implement a new publish track handler derivative publish_fetch_handler, like how we have a fetch_handler that implements subscribe. This uses a different bind in the server (maybe it could be in transport) with a publish function that does the right thing.

This seems to work, but I'm reasonably new to this code so please help.

Also bear in mind that I'm trying to walk the line between not a hacky mess but in time for interop tomorrow 😂

(min(stream type) = 1) + (min(fetch_header) = 1) = 2
@RichLogan RichLogan added the not ready Do not merge this pull request yet as it is not ready label Mar 14, 2025
@RichLogan
Copy link
Contributor Author

RichLogan commented Mar 14, 2025

I didn't actually do the message encoding. Let me go do that 😅 Updated.

@RichLogan RichLogan removed the not ready Do not merge this pull request yet as it is not ready label Mar 14, 2025
@RichLogan RichLogan changed the title Deliver FETCH data per the spec Deliver fetch data on fetch stream Mar 14, 2025
@RichLogan
Copy link
Contributor Author

RichLogan commented Mar 14, 2025

If this gets merged, a reminder to add an issue that group order fetch is not implemented.

@@ -42,7 +42,7 @@ namespace quicr {
/**
* Minimum bytes needed to write before considering to send. This doesn't
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a part of this review, but the comment is incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 it's actually confusing because this check is on receive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants