-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added support for policy identifier #130
base: main
Are you sure you want to change the base?
Conversation
I'm a big fan of this overall, I think it's a solid improvement in terms of consistency and ease of parsing. I am curious about the reasoning for separate |
RateLimit-Limit: 12 | ||
RateLimit-Policy: 12;w=1 | ||
RateLimit-Remaining: 6 ; using 50% of throughput, that is 6 units/s | ||
RateLimit-Reset: 1 | ||
~~~ | ||
|
||
If this is the case, the optimal solution is to achieve | ||
|
||
~~~ example | ||
RateLimit: limit=12, \ | ||
remaining=1 \ ; using 100% of throughput, that is 12 units/s | ||
reset=1 | ||
RateLimit-Limit: 12 | ||
RateLimit-Policy: 12;w=1 | ||
RateLimit-Remaining: 1 ; using 100% of throughput, that is 12 units/s | ||
RateLimit-Reset: 1 |
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.
Why did these get reverted to the older style?
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.
An error. I will fix.
~~~ | ||
|
||
the key value is the one referencing the lowest limit: `100` | ||
the value "sliding" identifies the policy being reported. | ||
|
||
11. Can we use shorter names? Why don't put everything in one field? |
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 question & answer should probably be revised.
The split is due to variability of the values and HTTP/2 HPACK compression. RateLimit-Policy should rarely change and therefore can be optimized by header compression. RateLimit will change on ever request and therefore will not be optimized. |
make returns this error.
|
@darrelmiller I tried to fix the broken section references: now it should build. Could you please PTAL? |
@nfriedly PR welcome for fixing inconsistencies :) |
There's a few instances still to be cleaned up:
|
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.
👋 hey folks - @kfcampbell shared this inside GitHub's API team as open for comment. I'm new to providing feedback on IETF proposals, so thank you for your patience if any of my comments are low-value or irrelevant to the crux of the PR 🙇♂️.
I've got a couple of points about wording consistency in this proposal, as well as some copyedits:
|
||
~~~ | ||
|
||
If a response contains both the Retry-After and the RateLimit header fields, the reset keyword value SHOULD reference the same point in time as the Retry-After field value. | ||
|
||
When using a policy involving more than one time window, the server MUST reply with the RateLimit header fields related to the time window with the lower remaining keyword values. | ||
When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower remaining keyword values. |
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.
When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower remaining keyword values. | |
When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower "r" parameter values. |
Does this need to be updated to be consistent with the change in https://github.com/ietf-wg-httpapi/ratelimit-headers/pull/130/files#diff-d26a6f407efe0e4913cbcf7bf0e2a164bee5edea8bf2fdc4aa97a7ceb3acc03cR229 (s/remaining keyword/r parameter/)?
There are other instances in this document that are still referring to remaining keyword
that I can't comment on because they're not part of the PR diff - do those need to be updated too?
More generally, I see some diffs where wording was changed from keyword
to parameter
, but keyword
remains in other areas. I'm new to this side of proposal drafts, so apologies if this is just bikeshedding 😁: should this wording be consistent throughout?
Co-authored-by: Johanan <jidicula@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.
Just adding two minor comments after reading the draft. ✌️ Overall, I like the draft so far -- great work! 👏
|
||
The field is a non-empty List of Items. Each item is a [quota policy](#quota-policy). | ||
Two quota policies MUST NOT be associated with the same quota units value. | ||
Two quota policies MUST NOT be associated with the same quota units value unless they are differentiated with a unique p parameter value. |
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.
Where is the meaning of parameter "p" defined? I don't see a definition for it nor an example using 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.
It is legacy. I have removed it.
Co-authored-by: Johanan <jidicula@github.com>
…httpapi/ratelimit-headers into darrelmiller-policyname
This proposal takes what we currently have in draft-07
and suggests the following alternative
This proposal introduces a policy "identifier" that connects the RateLimit field to the RateLimit-Policy field. The primary purpose is to address #127.
With the change in draft-07 to use structured fields in the header we now have the opportunity to explore different approaches and features without trying to maintain similarity with past efforts.