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 HttpConfiguration to Jetty server #27

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

ohadza
Copy link
Contributor

@ohadza ohadza commented Mar 14, 2021

No description provided.

@ohadza ohadza requested a review from asafalima March 14, 2021 15:36
@coveralls
Copy link

coveralls commented Mar 14, 2021

Coverage Status

Coverage increased (+1.3%) to 90.132% when pulling dac39f2 on support-jetty-http-configuration into b6ccf19 on master.

@asafm asafm self-assigned this Mar 15, 2021
Copy link

@asafm asafm left a comment

Choose a reason for hiding this comment

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

Let's also create a release - change version to 0.13

@@ -53,7 +54,7 @@ public void stop() throws Exception {
private void configureServer() {
List<ServerConnectorConfiguration> serverConnectorConfigurations = jerseyConfiguration.getServerConnectors();
serverConnectorConfigurations.forEach(configuration -> {
ServerConnector connector = new ServerConnector(server);
ServerConnector connector = new ServerConnector(server, new HttpConnectionFactory(configuration.getHttpConfiguration()));
Copy link

Choose a reason for hiding this comment

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

let's do this like the rest:

connector.setConnectionFactories(new HttpConnectionFactory(configuration.getHttpConfiguration()))

@@ -38,6 +39,11 @@ public JerseyConfigurationBuilder addPort(int port) {
return this;
}

public JerseyConfigurationBuilder addPortWithHttpConfiguration(int port, HttpConfiguration httpConfiguration) {
Copy link

Choose a reason for hiding this comment

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

It's too many flavors. Let's create a ServerConnectorConfigurationBuilder and do this right

@@ -38,7 +39,7 @@ public static void createServerAndTest(JerseyConfigurationBuilder configurationB
JettyServerCreator jettyServerCreator,
Tester tester) throws Exception {
int port = FreePortFinder.findFreeLocalPort();
configurationBuilder.addPort(port);
configurationBuilder.addPortWithHttpConfiguration(port, new HttpConfiguration());
Copy link

Choose a reason for hiding this comment

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

Let's add a small test that shows I configure jetty with it and it works. Max request header sounds about right

@ohadza ohadza requested a review from asafm March 15, 2021 22:00
@asafm asafm merged commit cfbd7c9 into master Mar 16, 2021
@asafm asafm deleted the support-jetty-http-configuration branch March 16, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants