Skip to content

Conversation

@jvgutierrez
Copy link
Member

As discussed yesterday on Slack, there are some scenarios, specially when proxy.config.http2.stream_priority_enabled is set to 0 where disabling HTTP/2 priority frames limit is interesting. This PR allows to disable it by setting proxy.config.http2.max_priority_frames_per_minute to 0

@maskit maskit added the HTTP/2 label Aug 28, 2019
@maskit maskit added this to the 10.0.0 milestone Aug 28, 2019
Copy link
Contributor

@sudheerv sudheerv left a comment

Choose a reason for hiding this comment

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

For consistency and completeness, I think we should've similar disable mechanism for rate limiting the Settings, Ping, Error Rate as well.

@bryancall
Copy link
Contributor

[approve ci autest]

bryancall
bryancall previously approved these changes Aug 28, 2019
@zwoop
Copy link
Contributor

zwoop commented Aug 28, 2019

@jvgutierrez Would you mind making similar changes for the other rate limiting settings ? If not, I'm ok landing this, and make a separate PR.

@zwoop
Copy link
Contributor

zwoop commented Aug 28, 2019

Also, it feels that if proxy.config.http2.stream_priority_enabled == 0, we ought to ignore the frames rather than rate limiting them ?

@jvgutierrez
Copy link
Member Author

Also, it feels that if proxy.config.http2.stream_priority_enabled == 0, we ought to ignore the frames rather than rate limiting them ?

PR updated, now the limit is only enforced if proxy.config.http2.stream_priority_enabled is set to 1, otherwise the frames are just ignored.

@jvgutierrez jvgutierrez force-pushed the allow-disabling-h2-priority-limit branch from be88436 to c798c39 Compare August 29, 2019 04:16
shinrich
shinrich previously approved these changes Sep 3, 2019
Copy link
Member

@shinrich shinrich 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 to me. Unfortunately, the PR needs to be rebased due to other merges causing conflicts.

@jvgutierrez
Copy link
Member Author

Looks good to me. Unfortunately, the PR needs to be rebased due to other merges causing conflicts.

done!

@maskit maskit merged commit f2ebbce into apache:master Sep 3, 2019
@zwoop
Copy link
Contributor

zwoop commented Oct 1, 2019

Cherry-picked to v9.0.x branch.

@zwoop zwoop modified the milestones: 10.0.0, 9.0.0 Oct 1, 2019
@zwoop zwoop modified the milestones: 9.0.0, 8.1.0 Mar 30, 2020
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.

6 participants