-
Notifications
You must be signed in to change notification settings - Fork 24
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
Joining Fetch #638
base: main
Are you sure you want to change the base?
Joining Fetch #638
Conversation
a407837
to
190c629
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Fixed some capitalization inconsistencies)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual Review:
Thanks for this. I think it's an improvement over "Current Group" in subscribe.
Thanks for the quick feedback @afrind! I'll fix up the definitions for how to resolve the range relative to the Subscribe so the terminology matches the current draft text and the details are more clear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good Mike. I added a few points for clarification.
draft-ietf-moq-transport.md
Outdated
|
||
* Previous Group Count: The number of groups to Fetch prior to the StartGroup of the corresponding Subscribe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there limits on this value? Is zero allowed? Is 1000 allowed? Is -1 allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidating discussion of this point.
For simplicity, we should disallow PGC = 0. It essentially overlaps the FETCH with the SUBSCRIBE. Forcing PGC >= 1 seems cleaner.
I'm in favor of allowing "Preceding Group Offset" = 0 so that we can have the option of using Fetch to retrieve the first portion of a group when Subscribe(LatestObject) is used to retrieve the rest. The updated text hopefully makes it clear that the Fetch and Subscribe do not overlap.
Is 1000 allowed?
Yes. Depending on how group ID are set for a given track this may make sense.
That said, it is not necessarily advisable to use a Joining Fetch to Fetch a large number of groups. In that case it may be better to wait for Subscribe OK and then make a series of smaller Fetch requests to fill the desired range.
I'm not opposed to putting some kind of limit on this value, but it's hard to say what it ought to be for all circumstances. I think it may be better to just allow publishers to reject joining fetch requests with PGOs larger than they wish to fulfill by responding with a Fetch Error. (I just created a separate issue about defining more error codes: #644)
Is -1 allowed?
No. This field is defined as [Preceding Group Offset (i),]
and this Variable-Length Integer Encoding does not permit negative values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual Review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to fix up the "what happens" when no content case but I like the overall PR here and seems to meet the use cases.
9dfceaf
to
a912b60
Compare
@ianswett let me know if there are any other editorial changes you'd like to see here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual Review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual Comments
Adopt Will's more concise language and add Alan's clarification about the edge case of tracks which haven't had any objects published to them yet. 1. moq-wg#638 (comment) 2. moq-wg#638 (comment)
- Adds Fetch Type field to Fetch - Defines pre-existing behavior as Fetch Type: Standalone Fetch - Defines new Fetch Type: Joining Fetch - Defines new error code: Invalid Subscribe ID
This name should be less confusing when considered in the context of potentially non-contiguous Group ID numbering
As discussed on the 2024-12-18 virtual interim call, this can simply result in a Fetch Error
Provide guidance rather than normative requirements for relay behavior in the cases where information is not (yet) available to determine the start of a Subscribe as discussed on the 2024-12-18 virtual interim call
replace non-normative 'may' with 'can'
Adopt Will's more concise language and add Alan's clarification about the edge case of tracks which haven't had any objects published to them yet. 1. moq-wg#638 (comment) 2. moq-wg#638 (comment)
efe4a5d
to
9fa2429
Compare
Use consistent spacing for this one case Magnus noticed to make it easier to search for the related terms. Not trying to be exhaustive since I think there are a lot of style/formatting consistency issues throughout the draft still and they should really be addressed together.
* Resolved Subscribe Start Object: | ||
* For Subscribes with Filter Type Latest Object, this is equal to Largest Object ID. | ||
* For Subscribes with Filter Type Latest Group, this is 0 | ||
* For Subscribes with Filter Type AbsoluteStart or AbsoluteRange, this is equal to the StartObject field of the Subscribe message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section seems to repeat the text to some extent in subscribe section. When PR #652 lands, some of these values may not match exactly especially when subscription filters are in the past to the live edge.
My proposal would be to refer to Subscribe section for these filter types and not repeat the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first wrote this with the relevant details collapsed into the below section and it was very verbose and hard to follow. I split it out by defining these intermediate "Resolved" values for clarity. I see your point about duplication, and you're right that #652 may also need to touch this, but I still prefer to state these things explicitly here to avoid confusion about how Joining Fetch values should be determined. Maybe @ianswett can weigh in as both editor and author of #652.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is #652 will invalidate some of the resolved values as it is defined in this PR. Given we had general agreement that Susbcribe needs updates if we add Joining Fetch, it would be nice to land these two PRs around the same time to keep things unambiguous.
draft-ietf-moq-transport.md
Outdated
* For Subscribes with Filter Type AbsoluteStart or AbsoluteRange, this is equal to the StartObject field of the Subscribe message | ||
* Preceding Group Offset: A field in the Joining Fetch message indicating the relative offset from the start of the Subscribe | ||
|
||
The Resolved Subscribe Start values for a Joining Fetch MUST correspond to the referenced Subscribe within the same session so that the Objects delivered across the Fetch and Subscribe are contiguous and non-overlapping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that we can guarantee "contigous" property but we can say it will be non-overlapping though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Joining Fetch it's important that the ranges of the Fetch and Subscribe portions at least be both contiguous and non-overlapping. Actual delivery of objects will of course be subject to whatever Fetch and Subscribe each provide. Perhaps I could reword slightly to keep this scoped to the properties we care about for Joining Fetch and not overstate anything with regard to delivery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By contiguous, if we mean groups will be in sequence with no gaps, we cannot guarantee that for variety of reasons. Am I misunderstanding it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think maybe we're focusing on different things here and I think it may relate to some of your other comments, too. I'm thinking of this primarily in terms of what is being requested - we can make our requests contiguous and non-overlapping, even if the way the objects are published and delivered means there could be gaps. What actually gets delivered will be subject to whatever limitations any other Subscribe and Fetch are subject to. I updated this line of the text to clarify that, too.
* Fetch Start Group: Resolved Subscribe Start Group - Preceding Group Offset | ||
* Fetch Start Object: 0 | ||
|
||
If Resolved Subscribe Start Object is 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Resolved Subscribe Start Object is 0/ Subscribe FilterType is LatestGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the only case here. This can also be true if the StartObject field of a Subscribe with Filter Type of AbsoluteStart or AbsoluteRange is 0. This is an example of why I'd prefer to retain that above section defining "Resolved Subscribe..." values rather than just referring directly to the Subscribe section's definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with having the section on Resolved subscribes , but seems bit incorrect to compute those absolute values given those are filters and may not map to the exact values that we mention here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, I'm thinking of this in terms of computing an absolute value for the requested range so we can delineate which portion would be provided by a Fetch (subject to whatever limitations a standalone Fetch is subject to) and which portion would be provided by a Subscribe (subject to whatever limitations a Subscribe is subject to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@englishm thanks for the PR. This is looking pretty good. I have put in few thoughts on some observations made. Please let me know your thoughts.
Accidentally added nearly identical text twice in moq-wg#581 - moq-wg@416f304 - moq-wg@08e64c8
without overstating (in this section at least) what will actually be delivered
Restores Group Order as configurable value for Joining Fetch by making it a field common to all Fetch Types See discussion: moq-wg#638 (comment)
We can just describe these values directly Thanks to Suhas for spotting this opportunity to tighten up this section moq-wg#638 (comment)
This PR adds the desired "Join" functionality as discussed at IETF 121 in Dublin1
Adding this functionality to Fetch unblocks further simplifications of Subscribe, allowing us to more clearly delineate between "past" (Fetch) and "future" (Subscribe). (see #598, #510 (review), etc.)
There are several ways we could allow a publisher to "atomically" join a Fetch and a Subscribe.
At IETF 121, @wilaw presented slides showing API options:
The consensus of the discussion seemed to heavily favor the latter design, so, as was requested, this PR is written as a modified form of Fetch: Joining Fetch.
This PR:
Footnotes
21:18 in the recording. Note: the Meetecho page includes the chat, synced to the video. ↩