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

Allow For Providing HttpConfiguration With Jetty #4032

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

isomarcte
Copy link
Member

This commit makes two meaningful changes,

  • Allow for the user to provide their own HttpConfiguration value. This is important as there are many settings which are not modeled in the JettyBuilder that one would general expect to have access to.
  • Enforces that the HttpConfiguration is used whether or not http2 is enabled. Formerly the HttpConfiguration was only used if there was not an SslConnectionFactory and http2 support was enabled.

This commit makes two meaningful changes,

* Allow for the user to provide their own `HttpConfiguration` value. This is important as there are many settings which are not modeled in the `JettyBuilder` that one would general expect to have access to.
* Enforces that the `HttpConfiguration` is used whether or not http2 is enabled. Formerly the `HttpConfiguration` was only used if there was not an `SslConnectionFactory` and http2 support was enabled.
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I like it.

I've thought it would be nice to provide an alternate builder that just took a Jetty instance and wrapped it in the typical server trappings. This would allow the more exotic parts of the API our builder doesn't cover. But I think this is good for what we do today.

@@ -400,6 +444,10 @@ object JettyBuilder {
case SSLClientAuthMode.Required =>
sslContextFactory.setNeedClientAuth(true)
}

/** The default [[org.eclipse.jetty.server.HttpConfiguration]] to use with jetty. */
val defaultJettyHttpConfiguration: HttpConfiguration =
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public? I guess we're already committed to supporting anHttpConfiguration on the API, so the additional liability isn't much. I could go either way here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rossabaker I made it public so that someone who wants to make a minor adjustment to the configuration can do so and then just use the rest of the "default" http4s configuration. Of course, since our default configuration right now is just the empty constructor, that doesn't really make a lot of sense.

I am going to make another PR which attempts to align the various configuration parameters which can be meaningfully shared between server backends, e.g. request header size etc. I'll make this private until that PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rossabaker updated

@rossabaker rossabaker added the enhancement Feature requests and improvements label Dec 18, 2020
@rossabaker rossabaker merged commit 36684fe into http4s:series/0.21 Dec 18, 2020
@isomarcte isomarcte deleted the expose-jetty-configuration branch December 18, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants