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

Editorial pass on explicit Path ID #334

Merged
merged 13 commits into from
May 23, 2024
Merged

Editorial pass on explicit Path ID #334

merged 13 commits into from
May 23, 2024

Conversation

mirjak
Copy link
Collaborator

@mirjak mirjak commented May 14, 2024

I mainly merged the "Path identifier" section into the right place in the rest of the document. Having such a separate section did repeat the same things multiple time. That is usually very unfortunate because that means you have to change things in multiple places.

Further, some smaller editorial issue in the rest of the document.

This is only a first pass, I think we need more iterations.

I mainly merged the "Path identifier" section into the right place in the rest of the document. Having such a separate section did repeat the same things multiple time. That is usually very unfortunate because that means you have to change things in multiple places.

Further, some smaller editorial issue in the rest of the document.

This is only a first pass, I think we need more iterations.
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
Copy link
Contributor

@huitema huitema left a comment

Choose a reason for hiding this comment

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

Looks good. Minor comments only.

draft-ietf-quic-multipath.md Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
Copy link
Contributor

@qdeconinck qdeconinck left a comment

Choose a reason for hiding this comment

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

This makes the draft a bit more readable. Some suggestions for typos + clarification purposes.

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
Co-authored-by: Quentin De Coninck <quentin.deconinck@umons.ac.be>
@Yanmei-Liu
Copy link
Contributor

Thanks a lot for all these work! Some minor suggestions.

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
Using multiple packet number spaces requires changes in the way AEAD is
applied for packet protection, as explained in {{multipath-aead}},
and tighter constraints for key updates, as explained in {{multipath-key-update}}.
The Path Identifier associated with the Destination Connection ID is used to
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems out of place here. Maybe just delete it?

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
@@ -543,22 +516,21 @@ MUST send a PATH_ABANDON frame to retire the Path ID.

### Allocating, Consuming and Retiring Connection IDs {#consume-retire-cid}

Each endpoints pre-allocate a Path Identifier for each new Connection ID.
Each endpoints pre-allocate a Path Identifier for each new connection ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's incorrect. Multiple CIDs are associated with the same path ID.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is correct because it doesn't say that it can be the same path ID. However, I agree it's confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not fully sure what to do here. Maybe just remove the sentence entirely?

Suggested change
Each endpoints pre-allocate a Path Identifier for each new connection ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest change the sentence into:
Each endpoints assign a Path Identifier for each new connection ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue it's the wrong way around anyway. First you create the path ID, then you issue CIDs for this path.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true. So it's better to change the sentence into:
'Endpoints first allocate unused Path Identifiers, then they issue Connection IDs for each Path Identifier.'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that is a useful sentence. I think we can just remove it. Or what do you think is missing if we remove it, @Yanmei-Liu ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is certainly necessary, we should clarify the relationship between CID and PATH ID at the very beginning of this section.

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
Comment on lines 555 to 556
The list of received packets used to send acknowledgements also remains
uneffected as the packet number space is associated with a path.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this confusing, and I'm surprised one would even consider this. This is just plain RFC 9000 behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this seems weird if you don't have any context about the single packet number space approach. However, does it really hurt to have this sentence?

I definitely planning for another editing pass, to stream-line the whole text. Maybe we can reconsider then.

Copy link
Contributor

Choose a reason for hiding this comment

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

in any case, "uneffected" --> "unaffected".

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
Thanks!

Co-authored-by: Marten Seemann <martenseemann@gmail.com>
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
Yanmei-Liu and others added 3 commits May 22, 2024 17:58
Co-authored-by: mirjak <mirja.kuehlewind@ericsson.com>
Co-authored-by: mirjak <mirja.kuehlewind@ericsson.com>
Co-authored-by: mirjak <mirja.kuehlewind@ericsson.com>
Copy link
Contributor

@qdeconinck qdeconinck left a comment

Choose a reason for hiding this comment

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

A few suggestions, but I would suggest merging this and addressing remaining concerns in sub issues.

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
mirjak and others added 2 commits May 23, 2024 16:17
Co-authored-by: Quentin De Coninck <quentin.deconinck@umons.ac.be>
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
@mirjak mirjak merged commit c142b9f into main May 23, 2024
3 checks passed
@mirjak mirjak deleted the mirjak-patch-1 branch May 28, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants