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

reinstate HttpChannel reuse in H2 #10868

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Nov 9, 2023

reinstate HttpChannel reuse in H2

@gregw gregw requested review from lorban and sbordet November 9, 2023 06:54
}

private HttpChannelOverHTTP2 pollHttpChannel()
public void setRecycleHttpChannels(boolean recycleHttpChannels)
Copy link
Contributor

Choose a reason for hiding this comment

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

This config needs to be wired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I took this from the commented out previous implementation. I think it can be permanently on and not wired into config. It is only there as an option in case we suspect a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if it's not wired, it's useless as there is no way a user can enable it, and we can easily remove this recycling logic locally if we ever suspect it's causing problems and are working on reproducing the issue.
I think this boolean should just go and have the queue logic always on.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lorban I think it needs to be restored, so it will be easier to performance test the effect of recycling channels.
For a benchmark we can subclass HTTP2ServerConnectionFactory.newConnection() and explicitly call the boolean setter on the HTTP2Connection.

@gregw
Copy link
Contributor Author

gregw commented Nov 12, 2023

@lorban I won't have time to land this one before vacation, so I'm "donating" it to you to complete.

@lorban
Copy link
Contributor

lorban commented Nov 13, 2023

@gregw sure, I'm taking over this PR.

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban marked this pull request as ready for review November 13, 2023 13:14
sbordet
sbordet previously approved these changes Nov 14, 2023
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet merged commit 4dd8bc9 into jetty-12.0.x Nov 14, 2023
7 of 8 checks passed
@sbordet sbordet deleted the fix/jetty-12.0.x/h2ReuseHttpChannels branch November 14, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants