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.

Sign up for GitHub

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

Jump to bottom

allow PRIORITY frames referring to placeholders exceeding SETTING_NUM_PLACEHOLDERS #2761

Closed
wants to merge 13 commits into from
Closed

allow PRIORITY frames referring to placeholders exceeding SETTING_NUM_PLACEHOLDERS #2761

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
231f8d9
allow PRIORITY frames referring to placeholders exceeding `SETTING_NU…
kazuho May 31, 2019
a87dd7a
s/0/zero/
kazuho May 31, 2019
4579ada
editorial
kazuho May 31, 2019
8328485
recommend >=32 placeholders
kazuho Jun 6, 2019
c379419
Update draft-ietf-quic-http.md
kazuho Jun 7, 2019
6b8b457
apply editorial suggestions from @MikeBishop as well as reorganizing …
kazuho Jun 7, 2019
af71abd
Merge branch 'master' into kazuho/placeholder-id-above-advertised-val…
kazuho Jun 9, 2019
18c8303
missing dependency is substituted by a orphan placeholder (instead of…
kazuho Jun 9, 2019
752df0d
add "for which", as suggested by @MikeBishop
kazuho Jun 12, 2019
d07c072
SHOULD at least 32, acknowledge that some might support zero
kazuho Jun 18, 2019
b60a6c5
fix grammar (credit to @rmarx)
kazuho Jun 18, 2019
06f0e8a
Update draft-ietf-quic-http.md
kazuho Jun 26, 2019
47abc99
editorial suggestion by @martinthomson
kazuho Jun 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion draft-ietf-quic-http.md
View file
Open in desktop
Expand Up @@ -641,7 +641,7 @@ client-controlled placeholders with an ID less than the value of this setting
with the confidence that the server will not have discarded the state. The
kazuho marked this conversation as resolved.
Show resolved Hide resolved
orphan placeholder cannot be prioritized or referenced by the client.

Servers are RECOMMENDED to support at least 32 placeholders. Those do not
Servers are RECOMMENDED to support at least 32 placeholders. Those that do not
kazuho marked this conversation as resolved.
Show resolved Hide resolved
SHOULD refrain from supporting placeholders at all, setting
`SETTINGS_NUM_PLACEHOLDERS` to zero.
Copy link
Member

Choose a reason for hiding this comment

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

I find this weird. I would strike the second sentence entirely and consider replacing it with justification for 32.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the outcome of the discussion (see above). The intent is:

  • recommend providing at least 32 placeholders
  • acknowledge that 0 is a sensible value when that not possible

The rational is to make it possible for a client to create a priority tree that works reasonably well, assuming that it uses no more than 32 placeholders in these two conditions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand that, but two chained SHOULD statements is hard to follow.

Mostly, I just don't agree with the conclusion of that discussion. If I want to provide 24, that is perfectly fine, and it might be enough for most clients. Pushing me toward advertising zero doesn't help at all.

So I don't agree with the idea that pushing people toward 0 is needed. They can make that decision on their own. And it's the default, so there is plenty of encouragement already.

An implementation that wants placeholders will have to implement some number of strategies for priority. If they get the number of placeholders they want, cool. If not, then they have to find another strategy.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ianswett was the main proponent for the "or bust" portion of this recommendation, I believe. Want to defend the sentence?


Expand Down