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

Add Dropwizard module #2236

Merged
merged 30 commits into from
Dec 23, 2019
Merged

Add Dropwizard module #2236

merged 30 commits into from
Dec 23, 2019

Conversation

OneCricketeer
Copy link
Contributor

@OneCricketeer OneCricketeer commented Nov 6, 2019

Adding module for dropwizard integration

Why

  1. I use Dropwizard at work, currently, I'm sure others do as well, and trust it's built-in Jetty http server, but might want other features. There already is a Dropwizard gRPC Bundle, but doesn't handle Thrift, AFAIK (not that that is a major feature for me, anyway).
  2. Wanted to see how difficult swapping out Dropwizard Jetty servers would be. 😄
  3. I am curious how much "faster" Dropwizard would be with Armeria. src: https://www.techempower.com/benchmarks/#section=data-r18&hw=ph&test=plaintext&l=zik0vz-f

Progress

  • Dropwizard Bundle
    • Buildable Source
    • Tests
      • YAML configurations
      • Dropwizard lifecycle of Armeria server
      • Testing of services within an Dropwizard application instance (maybe via AccessLog capture)
  • Runnable Example
  • Docs

Closes #2165

@CLAassistant
Copy link

CLAassistant commented Nov 6, 2019

CLA assistant check
All committers have signed the CLA.

@trustin
Copy link
Member

trustin commented Nov 6, 2019

Whoa. This is awesome! Let me review some time tomorrow!

Copy link
Contributor Author

@OneCricketeer OneCricketeer left a comment

Choose a reason for hiding this comment

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

Notes

@OneCricketeer OneCricketeer changed the title [WIP] Feature/dropwizard integration [WIP] Dropwizard Module Nov 6, 2019
@OneCricketeer
Copy link
Contributor Author

Master builds failing?

Task :examples:spring-boot-minimal:compileTestJava FAILED

@trustin
Copy link
Member

trustin commented Nov 7, 2019

Master builds failing?

That is weird. Build passes at master. Let me check what is happening in this branch.

@trustin
Copy link
Member

trustin commented Nov 7, 2019

My guess is that dropwizard-bom includes other dependencies than dropwizard-, interfering with Armeria's dependencies, such as Spring Boot 2. Could you remove dropwizard-bom and specify individual dependencies?

@OneCricketeer
Copy link
Contributor Author

Am seeing this Exception when running my example module with common AccessLogWriter

WARN  [2019-11-07 03:52:13,183] com.linecorp.armeria.common.logging.RequestLogListenerInvoker: onRequestLog() failed with an exception:
! java.lang.ClassCastException: class com.linecorp.armeria.server.HttpServerHandler$EarlyRespondingRequestContext cannot be cast to class com.linecorp.armeria.server.ServiceRequestContext (com.linecorp.armeria.server.HttpServerHandler$EarlyRespondingRequestContext and com.linecorp.armeria.server.ServiceRequestContext are in unnamed module of loader 'app')
! at com.linecorp.armeria.server.logging.AccessLogger.write(AccessLogger.java:109)
! at com.linecorp.armeria.server.logging.AccessLogWriter.lambda$common$0(AccessLogWriter.java:38)
! at com.linecorp.armeria.common.logging.RequestLogListenerInvoker.invokeOnRequestLog(RequestLogListenerInvoker.java:38)
! at com.linecorp.armeria.common.logging.DefaultRequestLog.addListener(DefaultRequestLog.java:346)
! at com.linecorp.armeria.common.logging.DefaultRequestLog.addListener(DefaultRequestLog.java:322)
! at com.linecorp.armeria.server.HttpServerHandler.lambda$respond0$6(HttpServerHandler.java:608)
! at io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:577)
! at io.netty.util.concurrent.DefaultPromise.notifyListeners0(DefaultPromise.java:570)
! at io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:549)
! at io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:490)

@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #2236 into master will increase coverage by 0.13%.
The diff coverage is 77.63%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2236      +/-   ##
============================================
+ Coverage     73.69%   73.83%   +0.13%     
- Complexity    10324    10430     +106     
============================================
  Files           895      910      +15     
  Lines         39405    39689     +284     
  Branches       4890     4902      +12     
============================================
+ Hits          29041    29303     +262     
- Misses         7882     7893      +11     
- Partials       2482     2493      +11
Impacted Files Coverage Δ Complexity Δ
...ecorp/armeria/dropwizard/ManagedArmeriaServer.java 0% <0%> (ø) 0 <0> (?)
...com/linecorp/armeria/dropwizard/ArmeriaBundle.java 0% <0%> (ø) 0 <0> (?)
...a/dropwizard/connector/ArmeriaServerDecorator.java 100% <100%> (ø) 4 <4> (?)
...opwizard/logging/CustomAccessLogWriterFactory.java 100% <100%> (ø) 5 <5> (?)
...om/linecorp/armeria/server/jetty/JettyService.java 63.05% <100%> (ø) 23 <0> (ø) ⬇️
...dropwizard/armeria/services/http/HelloService.java 100% <100%> (ø) 3 <3> (?)
...wizard/logging/CombinedAccessLogWriterFactory.java 100% <100%> (ø) 2 <2> (?)
...a/example/dropwizard/resources/JerseyResource.java 100% <100%> (ø) 2 <2> (?)
...ple/dropwizard/DropwizardArmeriaConfiguration.java 100% <100%> (ø) 1 <1> (?)
.../connector/proxy/ArmeriaProxyConnectorFactory.java 100% <100%> (ø) 7 <7> (?)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 150aa5a...788611c. Read the comment docs.

@trustin
Copy link
Member

trustin commented Nov 7, 2019

Am seeing this Exception when running my example module with common AccessLogWriter

Doh! Let me send a fix PR for that.

@minwoox
Copy link
Member

minwoox commented Nov 7, 2019

Oops, that was recently introduced by me. #2159 😅

@OneCricketeer
Copy link
Contributor Author

Now getting NoSuchMethodError related to the Guava relocation.

> Task :dropwizard:shadedTest
com.linecorp.armeria.server.dropwizard.ArmeriaServerFactoryTest > typesAreDiscoverable() FAILED
    java.lang.NoSuchMethodError: io.dropwizard.jackson.DiscoverableSubtypeResolver.getDiscoveredSubtypes()Lcom/linecorp/armeria/internal/shaded/guava/collect/ImmutableList;
        at com.linecorp.armeria.server.dropwizard.ArmeriaServerFactoryTest.typesAreDiscoverable(ArmeriaServerFactoryTest.java:45)

com.linecorp.armeria.server.dropwizard.ArmeriaConnectorFactoryTest > typesAreDiscoverable() FAILED
    java.lang.NoSuchMethodError: io.dropwizard.jackson.DiscoverableSubtypeResolver.getDiscoveredSubtypes()Lcom/linecorp/armeria/internal/shaded/guava/collect/ImmutableList;
        at com.linecorp.armeria.server.dropwizard.ArmeriaConnectorFactoryTest.typesAreDiscoverable(ArmeriaConnectorFactoryTest.java:49)

com.linecorp.armeria.server.dropwizard.ArmeriaConnectorFactoryTest > com.linecorp.armeria.server.dropwizard.ArmeriaConnectorFactoryTest$ArmeriaHttpsConnectorFactoryTest > withoutKeyStore_shouldNotValidate() FAILED
    java.lang.NoSuchMethodError: io.dropwizard.configuration.ConfigurationValidationException.getConstraintViolations()Lcom/linecorp/armeria/internal/shaded/guava/collect/ImmutableSet;
        at com.linecorp.armeria.server.dropwizard.ArmeriaConnectorFactoryTest$ArmeriaHttpsConnectorFactoryTest.withoutKeyStore_shouldNotValidate(ArmeriaConnectorFactoryTest.java:85)

com.linecorp.armeria.server.dropwizard.logging.AccessLogWriterFactoryTest > typesAreDiscoverable() FAILED
    java.lang.NoSuchMethodError: io.dropwizard.jackson.DiscoverableSubtypeResolver.getDiscoveredSubtypes()Lcom/linecorp/armeria/internal/shaded/guava/collect/ImmutableList;
        at com.linecorp.armeria.server.dropwizard.logging.AccessLogWriterFactoryTest.typesAreDiscoverable(AccessLogWriterFactoryTest.java:44)

11 tests completed, 4 failed

> Task :dropwizard:shadedTest FAILED

@trustin
Copy link
Member

trustin commented Nov 7, 2019

Now getting NoSuchMethodError related to the Guava relocation.

Ah, we need to exclude dropwizard packages for ShadowTask Gradle task. Let me send you a patch for this. Please stay in your IDE meanwhile. 🙇‍♂️

@trustin
Copy link
Member

trustin commented Nov 8, 2019

@Cricket007 A little bit more complicated than imagined, but here's the fix:

diff --git dropwizard/build.gradle dropwizard/build.gradle
index 099afb9a2..12dfd7bd9 100644
--- dropwizard/build.gradle
+++ dropwizard/build.gradle
@@ -1,3 +1,12 @@
+buildscript {
+    repositories {
+        gradlePluginPortal()
+    }
+    dependencies {
+        classpath "com.github.jengelman.gradle.plugins:shadow:${managedVersions['com.github.jengelman.gradle.plugins:shadow']}"
+    }
+}
+
 dependencies {
     compile project(':jetty')
     // Dropwizard
@@ -9,3 +18,16 @@ dependencies {
 
     testCompile 'io.dropwizard:dropwizard-testing'
 }
+
+// Do not relocate Guava because it's part of Dropwizard's public API.
+[tasks.shadedJar, tasks.shadedTestJar].each { task ->
+    task.relocators.clear()
+    project.ext.relocations.each { Map<String, String> props ->
+        def from = props['from']
+        def to = props['to']
+        if (from in ['com.google.common', 'com.google.thirdparty.publicsuffix']) {
+            return
+        }
+        task.relocate from, to
+    }
+}

dropwizard/README.md Outdated Show resolved Hide resolved
@trustin
Copy link
Member

trustin commented Nov 8, 2019

I'm still looking into fixing the ClassCastException you mentioned. Please ignore it meanwhile.

@trustin
Copy link
Member

trustin commented Nov 13, 2019

The ClassCastException has been fixed in master.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Just some nits.

@trustin
Copy link
Member

trustin commented Nov 25, 2019

Didn't have a chance to look into the error yet. Will try this week. 🙇‍♂️

OneCricketeer and others added 8 commits December 21, 2019 03:10
Signed-off-by: Jordan Moore <crikket.007@gmail.com>
Signed-off-by: Jordan Moore <crikket.007@gmail.com>
Signed-off-by: Jordan Moore <crikket.007@gmail.com>
Signed-off-by: Jordan Moore <crikket.007@gmail.com>
Signed-off-by: Jordan Moore <crikket.007@gmail.com>
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @Cricket007. You did an excellent work!

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks a million! You did a great job! 👍

@OneCricketeer
Copy link
Contributor Author

Thanks again, everyone!

@trustin trustin added this to the 0.98.0 milestone Dec 23, 2019
@trustin trustin changed the title Dropwizard Module Add Dropwizard module Dec 23, 2019
@trustin trustin merged commit db47ad3 into line:master Dec 23, 2019
@trustin
Copy link
Member

trustin commented Dec 23, 2019

🎉🎉🎉

Thanks, @Cricket007!

ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure moduleffers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and * `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure moduleffers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and * `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure moduleffers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and * `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure moduleffers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and * `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure moduleffers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and * `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure moduleffers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and * `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
ikhoon added a commit to ikhoon/armeria that referenced this pull request Jan 4, 2020
Motivation:
Armeria's Spring Boot auto configure module offers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different.
Sometimes it may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
* ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
trustin pushed a commit that referenced this pull request Jan 13, 2020
Motivation:
Armeria's Spring Boot auto configure module offers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by #2236. It can also customize Armeria Server from YAML.
But the configuration style is different between Spring Boot and Dropwizard.
The user should learn how to configure Armeria in each module. It may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
  ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`.
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
@OneCricketeer OneCricketeer deleted the feature/dropwizard-integration branch February 13, 2020 22:14
@minwoox minwoox mentioned this pull request Aug 21, 2020
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

Dropwizard is another popular framework we'd like to integrate Armeria into.

Modifications:

- Add `armeria-dropwizard` which provides a Dropwizard bundle and SPI implementations.
- Add documentation about Dropwizard integration
- Add an example

Result:

Closes line#2165
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:
Armeria's Spring Boot auto configure module offers to customize the Armeria server from JSON YAML.
Dropwizard module has been added to Armeria by line#2236. It can also customize Armeria Server from YAML.
But the configuration style is different between Spring Boot and Dropwizard.
The user should learn how to configure Armeria in each module. It may confuse the user.

Modification:
* Apply Spring Boot's YAML convention to the Dropwizard module.
* Add ArmeriaConfigurationUtil for applying ArmeriaSettings to
  ServerBuilder.
* Copy `CustomAliasKeyManagerFactory` and `CustomAliasX509ExtendedKeyManager` from
 `amreria/spring/boot-autoconfigure` to configure TLS fluenltly.
* Add meter exporsition for `DropwizardMeterRegistry`.
* Remove unused configuration files.

Result:
Provide a consistent configuration to Armeria Dropwizard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementing Armeria GRPC in Dropwizard?
5 participants