Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Priority and Group Send Order to SUBSCRIBE, clarify Publisher Priority #470
Add Priority and Group Send Order to SUBSCRIBE, clarify Publisher Priority #470
Changes from 3 commits
e43f244
0d45b9d
c1346ec
31b0ba2
0b693ae
f1b88f4
f5043c6
3c0f6e5
9097d73
81c6a32
9fc4721
619bc11
436605f
081554d
a085820
441ecf0
a97a91f
7ddfd11
1bd6788
c04f70e
6804eca
8d41133
b6ec889
73376b3
13d2e4c
751c65e
a6ed5ef
a2a29de
09e6d8e
74ca340
e32a488
4da15a1
779ddae
cc451f2
d366503
37fa963
0d351ff
c528c9c
fd21a0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is only true when groups have the same priority, since we're allowing the publisher priority to change per stream.
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 seems slightly under specified. If a last mile relay received two different values in both SUBSCRIBE and SUBSCRIBE_OK, we need to say which one takes priority. We agreed on the end subscriber would take priority so this should have a bit more text to say value in SUBSCRIBE overrides in value in SUBSCRIBE_OK.
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.
Good point, I rewrote this paragraph to better match the current framing.
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 wonder if we should add the note on publisher preference here too
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 : what's your suggestion? Something like:
In the absence of other specific reasons, relays SHOULD give all upstream subscriptions equal priority
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'm hesitant to spend text telling relays what to do, since they'll do what they want in reality. Telling them what not to do because it's a footgun seems more valuable.
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 expanded the sentence a bit more to note that a relay can use a single Subscriber Priority value for all upstream subscriptions.
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.
A proxy is a relay too.
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 intent of this sentence is to make it clear that they're different. Which I think is useful, because proxies typically have substantially different functionality.
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 can't a relay be a 1:1 relay? I'd just chop the sentence at
The recently merged terminology says
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 gets a bit off topic, but I think the more important property of a relay is that it republishes tracks from an upstream publisher without modification. I think there may be another entity we could define which could consume one or more tracks from upstream publishers and act as the original publisher for some new tracks (e.g. in the case of "manifest manipulation" use cases at the edge), but I think that's something we should discuss separately.
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 for this bit of text, I'm fine with it either way. To me, it makes explicit that relays may optionally be relatively stateless fan out nodes without any significant local caches, so long as they still allow downstream subscribers to subscribe to tracks from an upstream publisher.
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.
Based on the definition, I'll shorten this sentence and clarify some other text.