Add support Jetty's System.inheritedChannel option #1410

Merged
merged 1 commit into from Jan 14, 2016

Projects

None yet

2 participants

@matsumana
Contributor

No description provided.

@arteam arteam commented on an outdated diff Jan 13, 2016
dropwizard-example/example.yml
@@ -24,6 +24,16 @@ database:
# type: http
# port: 8080
+# run on a single port and server.connector.inheritChannel=true if your application launch by the "Server::Starter"
@arteam
arteam Jan 13, 2016 Member

I believe it would be better to document this option in the configuration reference instead of the example configuration file. We prefer to keep it simple for users who start with Dropwizard.

@arteam arteam commented on the diff Jan 13, 2016
...in/java/io/dropwizard/jetty/HttpConnectorFactory.java
@@ -185,6 +185,8 @@ public static ConnectorFactory admin() {
private String bindHost = null;
+ private boolean inheritChannel = false;
@arteam
arteam Jan 13, 2016 Member

Could you add a description of this option in HttpConnectorFactory javadoc? It would be consistent with the rest of options, which are documented.

@arteam
Member
arteam commented Jan 13, 2016

Thanks for contributing! I've added some comments about documentation, but in general the patch looks good. Could you update the release notes? This would make easier for users to track this change. Please, also squash your commits into a single commit.

@arteam arteam added the improvement label Jan 13, 2016
@arteam arteam added this to the 1.0.0 milestone Jan 13, 2016
@matsumana
Contributor

I fixed it.
Please review.

@arteam arteam commented on an outdated diff Jan 14, 2016
docs/source/manual/configuration.rst
@@ -277,6 +277,8 @@ Name Default Description
======================== ================== ======================================================================================
port 8080 The TCP/IP port on which to listen for incoming connections.
bindHost (none) The hostname to bind to.
+inheritChannel false If your application launch by the `Server::Starter`_, please set ``true``.
@arteam
arteam Jan 14, 2016 Member

I would prefer the description to be a little more generic. What do you think about this?

Whether this connector uses a channel inherited from the JVM. Use it with Server::Starter, to launch an instance of Jetty on demand"

Also the property need to be set in the HTTP connector YAML configuration.

@matsumana
Contributor

I agree.
Please review again.

@arteam arteam commented on an outdated diff Jan 14, 2016
...in/java/io/dropwizard/jetty/HttpConnectorFactory.java
@@ -50,6 +50,11 @@
* <td>The hostname to bind to.</td>
* </tr>
* <tr>
+ * <td>{@code inheritChannel}</td>
+ * <td>false</td>
+ * <td>If your application launch by the <a href="https://github.com/kazuho/p5-Server-Starter">Server::Starter</a>, please set 'true'</td>
@arteam
arteam Jan 14, 2016 Member

Please, update the Javadoc too. It should be consistent with the reference docs.

@arteam
arteam Jan 14, 2016 Member

Also, please don't forget to rebase and squash your commits.

@matsumana matsumana Add support Jetty's System.inheritedChannel option
86006b7
@matsumana
Contributor

I fixed it.
Please review again.

@arteam arteam merged commit cf19b00 into dropwizard:master Jan 14, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 78.791%
Details
@arteam
Member
arteam commented Jan 14, 2016

Thanks, @matsumana, for patience and thorough contribution!

@matsumana
Contributor

Thank you for merge!
When will 1.0.0 be released?

@arteam
Member
arteam commented Jan 15, 2016

I can't give you a strict ETA, because 1.0.0 is still being actively developed. But major features have already been contributed, so, I think, we could make a release candidate in the end of January. I will discuss this with the rest of the core team.

For the time being, you can use Dropwizard snapshots from the Sonatype's OSS repository. We publish them automatically after every succesfull build in the master branch.

Thank you for using Dropwizard.

@matsumana
Contributor

I see. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment