-
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
Delivery Order (aka Fetch lives) #452
Conversation
Takes an opinionated approach to adding 'Fetch' style functionality within the existing mechanisms. Can be improved once various TTL/etc PRs land. For example, we can add a 3rd mode where In Order is preferred within a client specified Delivery Timeout. (#449). Pulls in concepts from #428 Fixes #269 Fixes #326 Fixes #350 Closes #421
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
over ranges of Objects. In this case, a publisher MAY reset the existing stream | ||
and start sending on a new one, with the same Subscribe ID. Publishers MUST NOT |
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.
The MAY + MUST NOT is somewhat confusing. I think you are trying to say the publisher MAY continue to send objects that are no longer in the subscription, or reset the stream and open a new one in order to skip them?
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.
Wonder why does this not apply for stream per group or stream per object cases.
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.
@suhasHere my read of this mode is that it overrides the publisher preference. If subscriber asks for in-order, then the track is delivered in a single stream regardless.
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! makes sense .. I missed that.
skip Objects otherwise. Because there is no explicit Group ID, publishers MUST | ||
send Objects with the 'End of Group' status to indicate the end of the Group. |
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.
Why do you say there is no explicit group id? Don't the objects in Stream-per-track contain the group ID?
Latest In Order (0x2): Objects that have not exceeded the 'Delivery Timeout' | ||
({{delivery-timeout}}) SHOULD be delivered in order. Objects can be delivered |
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.
Can you clarify the expected behavior when a relay has a gap in their cache? eg: The relay has object 1, 2 and 4.
My read is that the relay MUST go upstream at least once to fill this gap, or get a tombstone for 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.
+1 to @afrind . we need some text to explain when to go upstream vs when to deliver what is there ? May be its already there and I am not seeing it ( so apologies if that's the case)
delivered by the same Track Alias and they would compete for bandwidth | ||
and other resources. When bandwidth is sufficient, a 'Latest In Order' and | ||
'Latest' subscription would be delivering the same Objects, because Objects |
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.
What about subscribes behind the live head? Wouldn't the two modes deliver the resources in opposite orders?
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.
To me, this is the key question, not just in this context, but re: caching expectations as a whole.
What behavior do we expect when a relay is serving active subscribers at the live edge, but a new subscribe comes in so far behind the live edge that all of the delivery timeouts have expired and none of the objects are in cache any longer? Then, what about the case where some objects haven't yet expired, but others have? Should that behavior be any different?
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.
Thanks for the PR. I feel this PR is hitting some of the cases we need.
I think this PR does need some more work to clarify few options - basically the interactions between priority across the delivery order options, what to do if there are gaps and cache duration. Relay upstreaming a subscription and what to pick or is it scoped to just last mile.
May be this is all some text clarification. But as-is I am not clear it addresses the issue completely.
send Objects with the 'End of Group' status to indicate the end of the Group. | ||
The 'Delivery Timeout' field is not present for this (0x1) mode. | ||
|
||
Latest In Order (0x2): Objects that have not exceeded the 'Delivery Timeout' |
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 "latest" do we mean, new groups will be prioritized before prior groups?
via OBJECT_STREAM, OBJECT_DATAGRAM, or STREAM_HEADER_GROUP streams, but | ||
MUST NOT change the delivery preference during a subscription. | ||
|
||
Latest (0x3): Objects from the most recent Group SHOULD be delivered first, |
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.
Latest here is more clear compared to the previous option.
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 should talk about this at the Seattle interim starting with use cases we are trying to solve.
It is really hard to review a proposal like this without understanding if / how this propagates upstream.
|
||
There are 3 delivery orders: | ||
|
||
Strict In Order (0x1): Delivers Objects in order of ascending Group Id and then |
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.
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 step back from this PR and talk about use cases and requirements with some face to face time. Right now I am strongly against merging this for a few reasons.
-
This eliminates the mode mode that is actually used the most in existing conferencing systems and the mode I think we need here
-
All the data I have see indicates that latest is not needed and performs worse that other approaches
-
Strict In Order seems like it would be extremely memory intensive to try and implement and I do not see the need for it. We have to buffer somewhere, clear the receiving clients, not the high scale relays, are the place to do that. This seems like it would introduce all kinds of DOS potential. System that want things to be in order inside a group will use stream per group and get this but the relays do not need to enforce it.
Most of these changes are in other PRs, so closing this. |
Takes an opinionated approach to adding 'Fetch' style functionality within the existing mechanisms.
Can be split up if people prefer.
Pulls in concepts from #411, #428 and #449 (Delivery Timeout)
Fixes #269
Fixes #326
Fixes #350
Fixes #396
Fixes #419
Fixes #431
Fixes #442
Relevant to #422, because we can omit the Object ID for 'In Order' delivery mode.
Closes #421
Closes #449