Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs: unhiding HTTP/3 and adding docs #15926
docs: unhiding HTTP/3 and adding docs #15926
Changes from 8 commits
4f190b3
bb5e934
28a6ea6
f20546f
5257f2f
0bad720
8ccb35a
1702e4c
7c9c5fa
f4ccb1d
d388cb9
4ec4357
a95b39c
e4151ae
76f04d6
c2496a3
62d053a
e10ad5f
58aa631
d2ad3ab
aa1f4ef
4fa9c31
323f151
56e08a6
bdb245f
752afc7
e2ee7a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
im wondering if this note should be a warning - and also thinking it might be better placed in docs/root/intro/arch_overview/http/http3.rst as its linked to from various places - feels a bit arbitrary to add it to one proto
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 lean towards the API because I think folks will actually read it here, where I'm less confident of folks reading the docs. Happy to make it a warning through.
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.
im wondering if this should make use of the
experimental
annotation that has just been addedagreed that there needs to be a warning here - but i would put the fuller explanation in the ref docs and point to that - as is done in most of the the api protos - mho
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 that "For example" is the start of a new sentence? If so, perhaps a period and then Capitalize "for". If it's not the start of a new sentence, perhaps it can be rephrased for clarity?
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.
nit: I feel like 'in-place' is how I am accustomed to seeing this phrase in the context of software. (As opposed to, say, "having everything in place"). Maybe? WDYT?
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.
nit: "requets" => "requests"
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
For Alt-Svc, I'd be inclined to say "HTTP Alternative Services" and I might also reference the spec https://tools.ietf.org/html/rfc7838
Is it worth mention the HTTPS DNS RR here as well, since we intend to implement this? (and the spec https://datatracker.ietf.org/doc/html/draft-ietf-dnsop-svcb-https-04)
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.
nit: I wonder if we can improve on the phrase "If HTTP/3 is configured via HTTP/3". How bout something like:
When HTTP/3 is configured, HTTP/3 connections will be attempted. However, the connection may fail over to TCP (HTTP/1 or HTTP/2 based on ALPN) instead. If the HTTP/3 connection fails explicitly because of a protocol error, a TCP connection will be attempted instead. In addition, if the HTTP/3 connection has not succeeded after a (currently) fixed timeout, a TCP connection will be started in a parallel. The connection the succeeds first will be used.
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.
Actually, having written all that... I think that's only true when H1, H2 and H3 are all enabled. If only H3 is enabled then there's no fallback and not alt-svc checking, right? Should we mention this? (Wow this gets complex in a hurry!)
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, for auto, remember that H2 and H1 are on by default, so you can't not configure 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.
Just for my curiosity, what is "auto config"?
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Totally your call... I wonder if we should be consistent about saying either "HTTP/2 or HTTP/3" vs "HTTP/2 or above". Probably not worth doing?
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 feel like in some situations it's awkward to phrase it the other way :-/
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 this text is identical to the HTTP/2 text (except 3 instead of 2), but I wonder if we should remove "the" after "utilizes"? "Whether the cluster utilizes http3 if configured..." WDYT?