-
Notifications
You must be signed in to change notification settings - Fork 20
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
SUBSCRIBE_NAMESPACE #498
SUBSCRIBE_NAMESPACE #498
Conversation
This message suite allows subscribers to express interest in ANNOUNCE/UNANNOUNCE for set of namespaces that match a prefix. To accomplish safe prefix matching, Track Namesapce is redefined from a single byte array to a N-Tuple of byte arrays. Fixes: #484
draft-ietf-moq-transport.md
Outdated
@@ -325,6 +331,20 @@ In MOQT, every track has a track name and a track namespace associated | |||
with it. A track name identifies an individual track within the | |||
namespace. | |||
|
|||
Track namespace is an N-tuple of bytes where N can be between 1 and 32. 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.
not sure if we need to limit the number of tuples. but we should instead have limit on the overall length.
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 need this to be "Ordered N-tuple" . From the definition of tuple from prog lang, it is sequence , but not sure if we need to make it clear.
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 limit on the number of tuples allows the relay to bound the complexity of a tree-structure used to implement tuple prefix matching.
Ordered tuple seems more specific, I'll update it.
draft-ietf-moq-transport.md
Outdated
* Track Namespace Prefix: An N-Tuple of byte fields which are matched against | ||
track namespaces known to the publisher. For example, if the publisher is a | ||
relay that has received ANNOUNCE messages for namespaces ("example.com", | ||
"meeting", "123", "participant=100") and ("example.com", "meeting", "123", |
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 example is bit misleading but the intent is fine. May be better way to say is
("example.com", "meeting/123", "participant/100")
The mix of a=b, vs split info is confusing.
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'll restructure the example to make it more clear.
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.
@afrind Huge thanks for this PR. This is coming along pretty well.
I do have few clarification questions and few details that, if added, will make it easy to reason about.
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 for the review.
draft-ietf-moq-transport.md
Outdated
@@ -325,6 +331,20 @@ In MOQT, every track has a track name and a track namespace associated | |||
with it. A track name identifies an individual track within the | |||
namespace. | |||
|
|||
Track namespace is an N-tuple of bytes where N can be between 1 and 32. 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.
The limit on the number of tuples allows the relay to bound the complexity of a tree-structure used to implement tuple prefix matching.
Ordered tuple seems more specific, I'll update it.
draft-ietf-moq-transport.md
Outdated
* Track Namespace Prefix: An N-Tuple of byte fields which are matched against | ||
track namespaces known to the publisher. For example, if the publisher is a | ||
relay that has received ANNOUNCE messages for namespaces ("example.com", | ||
"meeting", "123", "participant=100") and ("example.com", "meeting", "123", |
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'll restructure the example to make it more clear.
Philosophical question - we now have a N-32 tuple of namespace components and then a track name that hangs off the end. What's the utility in that? Could not the track name just be one of the namespace components? Can we simplify MOQT by getting rid of the concept of namespace and instead having a N-32 tuple track name? |
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.
Regarding Will's question:
Can we simplify MOQT by getting rid of the concept of namespace and instead having a N-32 tuple track name?
This also crossed my mind. At present, ANNOUNCE only announces namespace and SUBSCRIBE still requires a FULL match on namespace to route through a relay, so the distinction between namespace and name is serving a purpose. I guess we could say the last component is always the track name. I was hesitant in any case to completely undo track naming in this PR.
This is a good start. I am sure we will fix aspects of it as we explore futher, but overall LGTM |
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 comments, but overall LGTM
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
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.
Like the direction of this and it makes a bunch of things much easier to do.
draft-ietf-moq-transport.md
Outdated
|
||
: Indicates that x is a tuple, consisting of a variable length integer encoded as | ||
described in ({{?RFC9000, Section 16}}), followed by that many variable length | ||
tuple fields, each of which are encoded as (b) above. |
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.
Tiny nit but I don't love this syntax for this. It would be hard for someone reading this out of the blue to figure this out. But agree with the encoding , just wish there was less confusing syntax to do this.
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 agree that this is non-obvious. Should we burn some characters and make it "x (tup)" or "x (tuple)"?
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'll try it with x (tuple) and see what folks think
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.
yah, I like that x(tuple) proposal. ( and of course I don't care deeply if we keep it like it is, either way works for me )
It would be good to also bring in the changes proposed in #508 |
I will try to take a stab at that but I'm worried the scope is going to blow up this PR, and we'd do better to handle that in a follow up. I mentioned this briefly in the interim, but this PR adds significant functionality that is currently not possible so I am biased towards an iterative approach. |
To be clear, I'm happy to land this and then evaluate other improvements like #508 subsequently. |
Looking at #508 later after lands seems fine to me |
I hope we can get agenda time to talk about this one at the next call. This add critical functionally to moq that I would really like to have included even if we still needed time and implementation experience to get all the details sorted. |
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 believe all comments have been addressed, except "should we make this a special track instead".
@@ -378,6 +384,20 @@ In MOQT, every track has a track name and a track namespace associated | |||
with it. A track name identifies an individual track within the | |||
namespace. | |||
|
|||
Track namespace is an ordered N-tuple of bytes where N can be between 1 and 32. | |||
The structured nature of Track Namespace allows relays and applications to | |||
manipulate prefixes of a namespace. Track name is a sequence of bytes. |
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.
manipulate prefixes of a namespace. Track name is a sequence of bytes. | |
match on prefixes of a namespace. Track name is a sequence of bytes. |
Or maybe there are other types of manipulation?
This message suite allows subscribers to express interest in ANNOUNCE/UNANNOUNCE for set of namespaces that match a prefix. To accomplish safe prefix matching, Track Namesapce is redefined from a single byte array to a N-Tuple of byte arrays.
Fixes #484
Closes #253