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

add a stream-id: assignable and readable. Closes #1010 #1018

Merged
merged 7 commits into from Jun 3, 2022

Conversation

mwelzl
Copy link
Contributor

@mwelzl mwelzl commented May 16, 2022

Closes #1010

@mwelzl mwelzl added the API label May 16, 2022
@philsbln
Copy link
Contributor

Does this make sense outside of a protocol-specific context?

I am aware that this is a different can of worms adding protocol specific parameters to clone, but this looks like a strange concept to add an Integer here that may have different side effects in QUIC and SCTP…

@mwelzl
Copy link
Contributor Author

mwelzl commented May 16, 2022

That's a very good question. This feature request came up in the QuIC mapping part of the discussion in Vienna, see: https://datatracker.ietf.org/meeting/113/materials/minutes-113-taps-01

Not sure what to do: keep it as it is, remove it, or add the protocol:

  • assign: "if protocol = x, then stream ID = y"
  • read: "stream ID is y, and your protocol is x"
    I have to say, I don't like this particular way, as it would complicate matters quite a bit. But how else to do it?

@britram
Copy link
Contributor

britram commented May 17, 2022

All of the multistreaming protocols we know about have an integer stream identifier.

I suppose a future protocol could use some sort of binary blob for identifying streams (which are... pedantically... representable as arbitrary-length integers)

@tfpauly
Copy link
Contributor

tfpauly commented May 17, 2022

Interim discussion: let's make Clone() and Initiate() take a group of protocol-specific parameters (optionally) which allow protocols like QUIC and SCTP to extend this.

@mwelzl
Copy link
Contributor Author

mwelzl commented May 19, 2022

I just made a very minimalist update, where "protocolSpecificProperties" is only a parameter of Clone(). Why not list it as readable properties, and why not as a parameter of Initiate, as we discussed?

  • What's the point of saying "there can be protocol-specific properties which are currently not defined, and they can be readable"? This is for a future spec to say, when it proposes a new transport property.
  • Regarding Initiate(), I think this should in fact not be a parameter to the Initiate Action itself, but a transport property to be supplied when creating a Preconnection. Again, this made me find that I would only write something like "There can also be transport properties that are not yet defined", which doesn't seem to be useful text.

In conclusion, I found that only the Clone() Action needs this as a new parameter. Everything else is for future mapping documents.

Thoughts?

Copy link
Contributor

@philsbln philsbln left a comment

Choose a reason for hiding this comment

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

Thank you for your analysis – I really like the minimalistic approach.

I would prefer a less editorial framing as the one proposed.

draft-ietf-taps-interface.md Outdated Show resolved Hide resolved
draft-ietf-taps-interface.md Outdated Show resolved Hide resolved
mwelzl and others added 2 commits May 20, 2022 09:28
Co-authored-by: Philipp S. Tiesel <philipp@tiesel.net>
Co-authored-by: Philipp S. Tiesel <philipp@tiesel.net>
draft-ietf-taps-interface.md Outdated Show resolved Hide resolved
@mwelzl
Copy link
Contributor Author

mwelzl commented May 20, 2022

Many thanks! I incorporated your suggestion, and made a tiny edit to it (behaviour => behavior, underlaying => underlying, and "stream or connection" instead of "stream/connection").

Copy link
Contributor

@philsbln philsbln left a comment

Choose a reason for hiding this comment

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

Thanks – I guess we have a fairly good solution.

@tfpauly tfpauly merged commit 40f49fd into master Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add stream id's to Connection metadata
4 participants