-
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
Clarify SUBSCRIBE_UPDATE #653
Conversation
This also fixes older text from when it was allowable to have multiple simultaneous subscriptions. Fixes #576
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
same or a later object than StartGroup and StartObject. | ||
an existing subscription. Subscriptions can only become more narrow, not wider, | ||
because an attempt to widen a subscription could fail. If Objects before the | ||
start or after the end of the current subscription are needed, a fetch might |
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/fetch/FETCH/ ?
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.
We refer to a subscription with lower case, so I'm not sure which is appropriate here?
draft-ietf-moq-transport.md
Outdated
either rule or if the subscriber specifies a Subscribe ID that does not exist | ||
within the Session. |
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 Subscribe ID can also refer to a FETCH - so maybe "if it doesn't reference a current subscription"
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.
Failing the session is dangerous -- if the publisher has sent SUBSCRIBE_DONE, it might have already tossed state for the subscribe ID
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.
Do either of you have a suggestion on better phrasing or is the current text 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.
looks good . Agree with @afrind observations
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.
Some small comments
same or a later object than StartGroup and StartObject. | ||
an existing subscription. Subscriptions can only become more narrow, not wider, | ||
because an attempt to widen a subscription could fail. If Objects before the | ||
start or after the end of the current subscription are needed, a fetch might |
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.
We refer to a subscription with lower case, so I'm not sure which is appropriate here?
draft-ietf-moq-transport.md
Outdated
either rule or if the subscriber specifies a Subscribe ID that does not exist | ||
within the Session. |
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.
Do either of you have a suggestion on better phrasing or is the current text ok?
This also fixes older text from when it was allowable to have multiple simultaneous subscriptions.
Fixes #576