-
Notifications
You must be signed in to change notification settings - Fork 41
Change SUBSCRIBE_UPDATE to REQUEST_UPDATE and expand ability to update #1332
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
Conversation
This creates a mechanism to refresh auth credentials for PUBLISH, SUBSCRIBE_NAMESPACE and PUBLISH_NAMESPACE. A side-effect is that it allows the subscriber to change the priority of a FETCH (we closed #1204 already but this would address it). If we decide to address #1270, this will help. Fixes: #1267 Fixes: #1204
|
Looks good to me. I noticed a few places still using "established subscription", "established upstream subscription", "established downstream subscription", and """established namespace subscription", and could not figure out how to suggest all the changes in this PR, and ended submitting #1333; sorry for that. See more explanations in #1333. |
Co-authored-by: Ye-Kui Wang <[email protected]>
suhasHere
left a comment
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 not sure if we want to remove Subscribe update . it has more semantics than just auth changes.
This PR doesn't remove SUBSCRIBE_UPDATE, it renames it. Then it uses the renamed verb in a couple other places. I prefer this to creating an identical message for updating other things (Request ID + Params). |
ianswett
left a comment
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.
One nit, but LGTM
|
Discussed in 11/17 Interim: Wg should move forward with this change. There was separate concern about a broader change that would move all requests to bidirectional streams. |
draft-ietf-moq-transport.md
Outdated
| or REQUEST_ERROR message indicating if the update was successful. When an | ||
| update is unsuccessful, the publisher MUST also terminate the subscription with | ||
| PUBLISH_DONE with error code `UPDATE_FAILED`. | ||
| The sender of a request can later send a REQUEST_UPDATE to modify it. The |
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 ambiguous when we use the term "request" here. The term request is not defined anywhere IIUC.
draft-ietf-moq-transport.md
Outdated
| PUBLISH_DONE with error code `UPDATE_FAILED`. | ||
| The sender of a request can later send a REQUEST_UPDATE to modify it. The | ||
| receiver of a PUBLISH can also send REQUEST_UPDATE to modify subscriber sent | ||
| parameters. |
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 receiver of a PUBLISH" is a subscriber. This conflicts with "to modify subscriber sent parameters."
May be you meant to say " publisher sent parameters" instead ?
draft-ietf-moq-transport.md
Outdated
| receiver of a PUBLISH can also send REQUEST_UPDATE to modify subscriber sent | ||
| parameters. |
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.
| receiver of a PUBLISH can also send REQUEST_UPDATE to modify subscriber sent | |
| parameters. | |
| receiver of a PUBLISH can also send REQUEST_UPDATE to modify publisher sent parameters. |
suhasHere
left a comment
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.
couple of nits, others looks fine to me.
ianswett
left a comment
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.
Still LGTM
This creates a mechanism to refresh auth credentials for PUBLISH, SUBSCRIBE_NAMESPACE and PUBLISH_NAMESPACE.
A side-effect is that it allows the subscriber to change the priority of a FETCH (we closed #1204 already but this would address it).
If we decide to address #1270, this will help.
Fixes: #1267
Fixes: #1204