-
Notifications
You must be signed in to change notification settings - Fork 41
Put SUBSCRIBE_NAMESPACE on a stream, make Namespaces and PUBLISH independent #1344
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
base: main
Are you sure you want to change the base?
Conversation
afrind
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.
What does the subscriber do with the write half of the bidi stream after the SUBSCRIBE_NAMESPACE? FIN, or leave open?
How does this impact UNSUBSCRIBE_NAMESPACE, if at all?
If we introduce REQUEST_UPDATE (#1332), where does that go?
draft-ietf-moq-transport.md
Outdated
| Type (i) = 0x6, | ||
| Length (16), |
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.
Aren't these 3 bytes completely redundant?
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 the response stream can have both NAMESPACE and PUBLISH_NAMESPACE_DONE so you need type (and also REQUEST_OK/ERROR, maybe?). Length feels useless here, but I can live with 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.
Yes, I think I need a type. The Length is a bit more obviously redundant, but that goes back to the TLV vs TV conversation. I think there's a good argument that this isn't on the control stream and one could use TV even if we're using TLV elsewhere.
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 stream is currently a mini control stream that supports:
REQUEST_OK
REQUEST_ERROR
NAMESPACE
PUBLISH_NAMESPACE_DONE
There's some nice code reusability there, enough that I can live with the length redundancy. The alternative is quite a bit more spec text, but I can live with that too if folks are concerned.
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 it's worth considering, but we can make it separable.
| request the current set of matching published namespaces and `Established` | ||
| subscriptions, as well as future updates to the set. | ||
| The subscriber sends a SUBSCRIBE_NAMESPACE control message on a new | ||
| bidirectional stream to a publisher to request the current set of matching |
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.
How do you distinguish the control stream from this 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.
It's a new bidi stream and it starts with the SUBSCRIBE_NAMESPACE 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.
So if the first stream I receive starts with SUBSCRIBE_NAMESPACE, I hold it pending one that starts with CLIENT_SETUP?
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.
Without 0RTT clearly described, which it isn't today, I'm unsure how that could happen?
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.
Yeah @afrind if right.
It depends on if the QUIC library returns streams in stream_id order, but any bidirectional stream should be buffered until the CLIENT_SETUP is processed. In fact, the same is true for any unidirectional streams if pipelining a PUBLISH is valid (debatable with max_request_id), but that's a lot easier to implement.
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, I understand the issue now. Yes, I can call out this potential issue.
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.
PTAL at 2465b19
draft-ietf-moq-transport.md
Outdated
| the Track Namespace Prefix that have not already been sent to this subscriber. | ||
| If the set of matching PUBLISH_NAMESPACE messages changes, the publisher sends | ||
| the corresponding PUBLISH_NAMESPACE or PUBLISH_NAMESPACE_DONE message. | ||
| The publisher will respond with REQUEST_OK or REQUEST_ERROR. If 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.
I think you mean to say these will come on the response half of the bidi 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.
True, I updated the text above. This text is fairly duplicative I think, but I guess we can move it in another PR?
draft-ietf-moq-transport.md
Outdated
| the corresponding PUBLISH_NAMESPACE or PUBLISH_NAMESPACE_DONE message. | ||
| The publisher will respond with REQUEST_OK or REQUEST_ERROR. If the | ||
| SUBSCRIBE_NAMESPACE is successful, the publisher will send matching | ||
| NAMESPACE messages on the response stream if they are requested. |
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 we need or want a mechanism to get only the track names in the same format as namespaces, or we think only PUBLISH is sufficient?
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 do want that, but I was trying to keep this small. Happy to add it into this PR if we're all sure we want it.
draft-ietf-moq-transport.md
Outdated
|
|
||
| * Parameters: The parameters are defined in {{version-specific-params}}. | ||
|
|
||
| ## NAMESPACE {#message-pub-ns} |
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.
| ## NAMESPACE {#message-pub-ns} | |
| ## NAMESPACE_NOTIFY {#message-pub-ns} |
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 don't have a strong opinion, but NAMESPACE is shorter
Save repeating the bytes in the namespace prefix
It doesn't do anything RESET_STREAM can't
I'm inclined to leave it open, at least until we decide if want to send REQUEST_UPDATE on it.
I removed it in the more recent update, because it wasn't needed.
On the open bidi stream. |
draft-ietf-moq-transport.md
Outdated
| Type (i) = 0x6, | ||
| Length (16), |
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 stream is currently a mini control stream that supports:
REQUEST_OK
REQUEST_ERROR
NAMESPACE
PUBLISH_NAMESPACE_DONE
There's some nice code reusability there, enough that I can live with the length redundancy. The alternative is quite a bit more spec text, but I can live with that too if folks are concerned.
| request the current set of matching published namespaces and `Established` | ||
| subscriptions, as well as future updates to the set. | ||
| The subscriber sends a SUBSCRIBE_NAMESPACE control message on a new | ||
| bidirectional stream to a publisher to request the current set of matching |
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.
So if the first stream I receive starts with SUBSCRIBE_NAMESPACE, I hold it pending one that starts with CLIENT_SETUP?
| request the current set of matching published namespaces and `Established` | ||
| subscriptions, as well as future updates to the set. | ||
| The subscriber sends a SUBSCRIBE_NAMESPACE control message on a new | ||
| bidirectional stream to a publisher to request the current set of matching |
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.
Yeah @afrind if right.
It depends on if the QUIC library returns streams in stream_id order, but any bidirectional stream should be buffered until the CLIENT_SETUP is processed. In fact, the same is true for any unidirectional streams if pipelining a PUBLISH is valid (debatable with max_request_id), but that's a lot easier to implement.
So it can use the Suffix approach and not specify the full Namespace
|
Discussed 11/13 with Authors/Editors -- plan to present to the wg at the interim |
|
Presented at 11/17 Interim: Plan is to proceed with this design. |
afrind
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.
This looks close
draft-ietf-moq-transport.md
Outdated
| part of that namespace. This includes echoing back PUBLISH or PUBLISH_NAMESPACE | ||
| messages to the endpoint that sent them. If an endpoint accepts its own |
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.
PUBLISH_NAMESPACE is not longer echoed?
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 it probably should be, to minimize any behavior change, but the PUBLISH_NAMESPACE would turn into a NAMESPACE message on the wire.
draft-ietf-moq-transport.md
Outdated
|
|
||
| The publisher sends the `NAMESPACE_DONE` control message to indicate its | ||
| intent to stop serving new subscriptions for tracks within the provided Track | ||
| Namespace. All NAMESPACE messages are in response to a SUBSCRIBE_NAMESPACE, |
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 bit about NAMESPACE messages applies to both NS and NS_DONE, and is in the NS_DONE section.
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.
Fair point, any suggestions? Just remove it?
draft-ietf-moq-transport.md
Outdated
| * Track Namespace Suffix: Specifies the final portion of a track's | ||
| namespace as defined in {{track-name}}. The namespace begins with the | ||
| 'Track Namespace Prefix' specified in {message-subscribe-ns}. |
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.
"final portion" is a bit under defined. Maybe
Specifies the final portion of a track's namespace as defined in {{track-name}} after removing namespace tuples included in Track Namespace Prefix...
Co-authored-by: afrind <[email protected]>
| Length (16), | ||
| Request ID (i), | ||
| Track Namespace Prefix (..), | ||
| Subscribe Options (i), |
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 one send 2 subscribe_namespace with 2 different options
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.
No, this falls under duplicate prefix text elsewhere.
| could arrive prior to the control stream, in which case the data SHOULD be buffered | ||
| until the control stream arrives and setup is complete. | ||
| If an implementation does not want to buffer, it MAY reset other bidirectional | ||
| streams before the session and control stream are established. |
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 PR does not really provide a good way for implementations to figure out what stream is the control 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.
There is a type identifier at the beginning of the control stream, which is the SETUP messages.
| |-------|-----------------------------------------------------| | ||
| | 0xE | NAMESPACE_DONE ({{message-namespace-done}}) | | ||
| |-------|-----------------------------------------------------| | ||
| | 0xC | PUBLISH_NAMESPACE_CANCEL ({{message-pub-ns-cancel}})| |
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.
It looks like we're repeating the mistake we had in HTTP/3 where we are adding streams without type identifiers and then have some semantic rules about what messages can appear where. Can we have an explicit type for bidi streams instead?
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.
@vasilvv the current PR is adequate, in that it either starts with CLIENT_SETUP or it starts with SUBSCRIBE_NAMESPACE. This is extensible since have varint frame type so have unlimited ways to start streams. It seems like it would be fairly redundant to have:
SETUP_STREAM
CLIENT_SETUP
and
SUB_NS_STREAM
SUB_NS
Right?
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 guess the counter-argument is if in the future you want to start a stream with a non-message. This is what webtransport ran into and it got a little ugly, but ultimately resolved it by reserving a frame type and giving it new semantics.
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 definitely don't want to repeat that mistake, but in this case we are adding type/message identifiers.
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 can see that this works, but I don't see why we would want to do that. If there are two types of streams, and they have basically disjoint message type list, why do we want them to have the same format?
| A SUBSCRIBE_NAMESPACE can be cancelled by closing the stream with | ||
| either a FIN or RESET_STREAM. Cancelling does not prohibit original publishers |
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 SUBSCRIBE_NAMESPACE can be cancelled by closing the stream with | |
| either a FIN or RESET_STREAM. Cancelling does not prohibit original publishers | |
| A SUBSCRIBE_NAMESPACE can be cancelled by the creator the stream, by closing the stream with | |
| either a FIN or RESET_STREAM. Cancelling does not prohibit original publishers |
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.
It can be cancelled by either the creator or the peer.
Co-authored-by: Suhas Nandakumar <[email protected]>
Co-authored-by: Suhas Nandakumar <[email protected]>
Co-authored-by: Suhas Nandakumar <[email protected]>
I did not remove Request ID entirely in this PR, because it makes the most sense to do that all at once, but I did create new 'NAMESPACE' and 'NAMESPACE_DONE' messages that are separate from PUBLISH_NAMESPACE/PUBLISH_NAMESPACE_DONE and much simpler.
Saves bytes on the wire by not repeating the 'Track Namespace Prefix' for every Namespace.
Removes UNSUBSCRIBE_NAMESPACE because we can just close the stream.
Fixes #1348
Fixes #1310
Fixes #1305
Fixes part of #1168
Fixes #843
Further possible changes: