Skip to content
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

Announce #643

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Announce #643

wants to merge 10 commits into from

Conversation

martinduke
Copy link
Contributor

Fixes #556
Fixes #557
Fixes #577
Fixes #583
Fixes #584

Also eliminates text from the relay section that is redundant, because it applies to endpoints whether or not they are relays.

@martinduke martinduke requested a review from ianswett December 13, 2024 19:39
Copy link
Collaborator

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still find some of the ANNOUNCE flow overly complex, but this does a good job of describing how you might use the defined messages.


If the subscriber is aware of a namespace of interest, it can send
SUBSCRIBE_ANNOUNCES to publishers/relays it has discovered. This message
increases the likelihood that publishers will send relevant ANNOUNCE messages
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it required to send ANNOUNCES that match the prefix?

Copy link
Contributor Author

@martinduke martinduke Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normative words are in the ANNOUNCE section. I had SHOULD under the general theory of "server's gonna do..." but I'll change it to MUST because I don't think it matters that much. The question is "MUST if what?" and my next edit will attempt to answer that.

draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
@ianswett ianswett added Announce Issues with Announce message and handling Design Issues or PRs that change how MoQ works including the wire format. labels Dec 17, 2024
Copy link
Collaborator

@afrind afrind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Individual Review:

Thanks for tackling this. There's a lot going on here.

Moving the error codes is helpful but seems like a great candidate for a standalone editorial PR.

There are some normative changes to relay handling not related to ANNOUNCE which should also be in a separate PR.

There's a subtle deletion of the "exact" namespace matching behavior. This is less problematic in the face of structured namespaces, but I forget if that was something we discussed and had consensus on?

increases the likelihood that publishers will send relevant ANNOUNCE messages
for that namespace.

An UNSUBSCRIBE_NAMESPACES negates the effect of a SUBSCRIBE_NAMESPACES. It does
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"negates the effect" has me reaching for a D20

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'withdraws a previous SUBSCRIBE_NAMESPACES' ?

draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
@martinduke
Copy link
Contributor Author

Individual Review:

Thanks for tackling this. There's a lot going on here.

Moving the error codes is helpful but seems like a great candidate for a standalone editorial PR.

This PR is already heavily editing the relay section for redundant and poorly located text. This falls into that theme; it is not specific to relays.

There are some normative changes to relay handling not related to ANNOUNCE which should also be in a separate PR.

Again, a huge rewrite of the Relay section is in scope for this.

There's a subtle deletion of the "exact" namespace matching behavior. This is less problematic in the face of structured namespaces, but I forget if that was something we discussed and had consensus on?

I don't see how structured namespaces can work with exact matching.

@martinduke martinduke mentioned this pull request Jan 10, 2025
Copy link
Collaborator

@afrind afrind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better. A bunch of editorial comments/suggestions. The substantive comment is about superset.

@@ -612,6 +612,105 @@ and announcements. The client can choose to delay closing the session if it
expects more OBJECTs to be delivered. The server closes the session with a
'GOAWAY Timeout' if the client doesn't close the session quickly enough.

# Track Discovery and Retrieval (#track-discovery}

Discovery of MoQT servers is always done out-of-band. Track discovery is done in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/is done/can be done/ ?

There are cases where track discovery is also out of band (eg catalog, hard coded in specifications), right?


Discovery of MoQT servers is always done out-of-band. Track discovery is done in
the context of an established MoQT session. The session client might be a
subscriber, publisher, or both.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The session client might be a subscriber, publisher, or both.

This is true, but is it related to track discovery?


Given sufficient out of band information, it is valid for a subscriber
to send a SUBSCRIBE or FETCH message to a publisher (including a relay) without
any previous MoQT messages besides SETUP. However, SUBSCRIBE_NAMESPACES and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUBSCRIBE_ANNOUNCES

Given sufficient out of band information, it is valid for a subscriber
to send a SUBSCRIBE or FETCH message to a publisher (including a relay) without
any previous MoQT messages besides SETUP. However, SUBSCRIBE_NAMESPACES and
ANNOUNCE messages provide an in-band means of discovery of subscribers and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can discover subscribers via these messages.

...discovery of publishers and track namespaces?

Aside: I've been thinking there might be value in an optional track name (or names) in ANNOUNCE, to assist with catalog-less discovery.


If the subscriber is aware of a namespace of interest, it can send
SUBSCRIBE_ANNOUNCES to publishers/relays it has discovered. This message
increases the likelihood that publishers will send relevant ANNOUNCE messages
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like 'increases the likelihood'. Maybe:

The recipient of the message will send any relevant ANNOUNCE messages for that namespace.

UNANNOUNCE. A publisher and subscriber might seek alternate subscribers and
publishers, respectively, for potential SUBSCRIBE and FETCH interactions.

The receiver of an ANNOUNCE_OK or ANNOUNCE_ERROR SHOULD report this to the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this section move closer to ANNOUNCE (above UNANNOUNCE/ANNOUNCE_CANCEL)

data stream is already open, it MAY send STOP_SENDING for the data stream along
with FETCH_CANCEL, but MUST send FETCH_CANCEL.

The Publisher can destroy subscription or fetch state as soon as it has received
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order you have is:

subscriber SUBSCRIBE state
subscriber FETCH state
publisher FETCH state
publisher SUBSCRIBE state

which I find dissatisfying. Can you reorder (either sub sub/sub fetch/pub sub/pub fetch, or sub sub/pub sub/sub fetch/pub fetch is fine)

has received an ANNOUNCE with a namespace that includes that track, it SHOULD
only request it from the senders of those ANNOUNCE messages.

The central interaction with a publisher is to send SUBSCRIBE and/or FETCH for
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reorder these to paragraphs?

@@ -1545,16 +1585,28 @@ message for which this response is provided.

* Reason Phrase: Provides the reason for announcement error.

The following error codes are defined:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to die on this hill, but moving the error code tables could easily go in another PR.

both endpoints can immediately destroy relevant state. Objects MUST NOT be sent
for requests that end with an error.

A publisher MUST send exactly one SUBSCRIBE_OK or SUBSCRIBE_ERROR in response to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move closer to the top of the section, before the state bits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Announce Issues with Announce message and handling Design Issues or PRs that change how MoQ works including the wire format.
Projects
None yet
4 participants