Provide a consistent configuration to Armeria Dropwizard #2373

ikhoon · 2020-01-04T18:22:00Z

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.

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 · 2020-01-06T06:16:38Z

Test failure:

com.linecorp.armeria.dropwizard.ArmeriaConfigurationUtilTest > configureServer() FAILED
    org.opentest4j.AssertionFailedError: 
    Expecting:
     <true>
    to be equal to:
     <false>
    but was not.
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at com.linecorp.armeria.dropwizard.ArmeriaConfigurationUtilTest.configureServer(ArmeriaConfigurationUtilTest.java:61)

trustin · 2020-01-06T06:16:49Z

@Cricket007 PTAL

ikhoon · 2020-01-06T06:20:11Z

com.linecorp.armeria.dropwizard.ArmeriaConfigurationUtilTest > configureServer() FAILED

Oops... let me take a look. 😅

OneCricketeer

Not really a fan of this redundancy

type: armeria
armeria: 
   ... settings...

If it cannot be flattened, that's fine

dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaConfigurationUtil.java

dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaServerFactory.java

OneCricketeer · 2020-01-06T06:41:10Z

My initial intention was to have a minimal rewrite from existing dropwizard http configuration to just replace the server type, not match any Spring config similarities

ikhoon · 2020-01-06T08:02:30Z

Not really a fan of this redundancy
type: armeria
armeria: 
   ... settings... 

I agreed, wanted to remove it but... I think we need own name to avoid conflict.

a-z seems rather broad, assuming just (kMGTP)?(Bb)

That is a nice suggestion. This is a fork of the original code.

armeria/spring/boot-autoconfigure/src/main/java/com/linecorp/armeria/internal/spring/ArmeriaConfigurationUtil.java

Line 107 in 9546b28

    
           private static final Pattern DATA_SIZE_PATTERN = Pattern.compile("^([+]?\\d+)([a-zA-Z]{0,2})$");

There are lots of duplication code between Dropwizard and Spring Boot configure.
After releasing 1.0 we plan to merge that APIs to provide a general interface. But now we decided to keep the code as equal as possible.

Let me add your review to TODO for further refactoring.

Simplify with Math.pow?

This one too.

My initial intention was to have a minimal rewrite from existing dropwizard http configuration to just replace the server type, not match any Spring config similarities

I understand your intention. As you mentioned in advanced-dropwizard-integration.rst, Armeria supports multiple protocols over the same port. I think the Dropwizard users could take advantage of that by this style configuration.

OneCricketeer · 2020-01-06T08:13:11Z

I think we need own name to avoid conflict.

I'm not sure there's a conflict, I simply mean move "up" all properties of the ArmeriaSettings class directly into the ServerFactory.

ikhoon · 2020-01-06T08:44:04Z

I'm not sure there's a conflict, I simply mean move "up" all properties of the ArmeriaSettings class directly into the ServerFactory.

Let me remove the armeria namespace. I'm working on now. :-)

@Cricket007

- Remove `armeria` namespace in ArmeriaServerFactory - Document `accessLog` property - Add TODO commented by @Cricket007

codecov · 2020-01-06T09:14:19Z

Codecov Report

Merging #2373 into master will decrease coverage by 0.17%.
The diff coverage is 73.34%.

@@             Coverage Diff              @@
##             master    #2373      +/-   ##
============================================
- Coverage     73.49%   73.32%   -0.18%     
- Complexity    10620    10690      +70     
============================================
  Files           928      934       +6     
  Lines         40845    41193     +348     
  Branches       5048     5142      +94     
============================================
+ Hits          30021    30206     +185     
- Misses         8272     8370      +98     
- Partials       2552     2617      +65

Impacted Files	Coverage Δ	Complexity Δ
...necorp/armeria/client/DecoratingClientFactory.java	`50% <ø> (ø)`	`6 <0> (ø)`	⬇️
...meria/common/metric/PrometheusMeterRegistries.java	`90.9% <ø> (-0.76%)`	`5 <0> (ø)`
...om/linecorp/armeria/common/logging/RequestLog.java	`23.52% <ø> (ø)`	`8 <0> (ø)`	⬇️
.../main/java/com/linecorp/armeria/common/Cookie.java	`28.57% <ø> (ø)`	`22 <0> (ø)`	⬇️
...ava/com/linecorp/armeria/server/RoutingResult.java	`63.46% <ø> (ø)`	`20 <0> (ø)`	⬇️
.../main/java/com/linecorp/armeria/client/Client.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...p/armeria/dropwizard/ArmeriaConfigurationUtil.java	`38.34% <ø> (-3.55%)`	`16 <0> (-1)`
...linecorp/armeria/common/AggregatedHttpMessage.java	`80% <ø> (ø)`	`4 <0> (ø)`	⬇️
...etrofit2/RetrofitMeterIdPrefixFunctionBuilder.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...com/linecorp/armeria/common/QueryParamGetters.java	`66.66% <ø> (ø)`	`2 <0> (ø)`	⬇️
... and 190 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 7738209...2f77a76. Read the comment docs.

trustin · 2020-01-06T13:25:34Z

Sorry about the conflict. 😅

ikhoon · 2020-01-06T13:26:54Z

Sorry about the conflict. 😅

No worries. Let me resolve it :-)

dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaConfigurationUtil.java

dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaSettings.java

- Remove metricsPath from ArmeriaSettings - Update Javadoc

minwoox

some nits. 😄

dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaConfigurationUtil.java

site/src/sphinx/advanced-dropwizard-integration.rst

OneCricketeer · 2020-01-07T19:39:48Z

Hey, btw, I noticed the docs aren't on the main page yet

ikhoon · 2020-01-07T23:52:48Z

Hey, btw, I noticed the docs aren't on the main page yet

Did you mean the Armeria official homepage?
https://line.github.io/armeria/
Our latest version is 0.97.0. We didn’t release 0.98.0 yet. It will be seen after releasing the next version. 😉

trustin

Nice work, @ikhoon !

minwoox

Oops forgot to approve this.
Thanks @ikhoon!

trustin · 2020-01-10T05:26:21Z

Any other comments, @Cricket007 ? Would like to pull this in once you approve.

OneCricketeer

Minor feedback. Good work, otherwise

dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaSettings.java

dropwizard/src/test/java/com/linecorp/armeria/dropwizard/TestApplication.java

OneCricketeer · 2020-01-10T06:06:29Z

dropwizard/src/main/resources/META-INF/services/io.dropwizard.jackson.Discoverable

@@ -1 +0,0 @@
-com.linecorp.armeria.dropwizard.logging.AccessLogWriterFactory


Is this (and related files) really not needed?

Or did you just remove the @JsonTypeInfo usages?

@JsonTypeInfo for AccessLogWriter has been removed, so I think we can remove it.
Because:

The AccessLogWriter type and format can be configured via AccessLog in ArmeriaSettings.

armeria/dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaSettings.java

Lines 919 to 955 in 3d6d805

/**

* Configurations for the access log.

*/

static class AccessLog {

/**

* The access log type that is supposed to be one of

* {@code "common"} {@code "combined"} or {@code "custom"}.

*/

@Nullable

private String type;

/**

* The access log format string.

*/

@Nullable

private String format;

@Nullable

String getType() {

return type;

}

void setType(String type) {

this.type = type;

}

@Nullable

String getFormat() {

return format;

}

void setFormat(String format) {

this.format = format;

}

}

}

The AccessLogWriter is set to ServerBuilder statically in configureAccessLog

armeria/dropwizard/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaConfigurationUtil.java

Lines 469 to 478 in 7413d19

private static void configureAccessLog(ServerBuilder serverBuilder, AccessLog accessLog) {

if ("common".equals(accessLog.getType())) {

serverBuilder.accessLogWriter(AccessLogWriter.common(), true);

} else if ("combined".equals(accessLog.getType())) {

serverBuilder.accessLogWriter(AccessLogWriter.combined(), true);

} else if ("custom".equals(accessLog.getType())) {

serverBuilder

.accessLogWriter(AccessLogWriter.custom(accessLog.getFormat()), true);

}

}

Let me know if there is something I missed.

Just making sure it wasn't needed anymore.

Ref. https://www.dropwizard.io/en/stable/manual/configuration.html#polymorphic-configuration

Personally, I think statically assigned if-else (or switch on string) are code-smells.

https://refactoring.guru/replace-conditional-with-polymorphism

- Remove unused code - Use MediaType constants for compresssion

trustin · 2020-01-13T06:39:07Z

Thanks a lot, @ikhoon and reviewers!

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.

ikhoon added the cleanup label Jan 4, 2020

ikhoon requested review from minwoox and trustin as code owners January 4, 2020 18:22

OneCricketeer reviewed Jan 6, 2020

View reviewed changes

Remove verboseResponses validation because of gradle systemProperty

842f337

Address comments by @Cricket007

1d6e732

- Remove `armeria` namespace in ArmeriaServerFactory - Document `accessLog` property - Add TODO commented by @Cricket007

ikhoon added 3 commits January 6, 2020 18:18

Move TODO location

9ed6f60

Remove moved TODO

7f45a3b

Update Javadoc and outdated YAML configuration

7738209

Merge branch 'master' into dropwizard-configure

ac73d7b

trustin reviewed Jan 7, 2020

View reviewed changes

ikhoon added 3 commits January 7, 2020 12:15

Address comments by @trustin, @Cricket007

69d8884

- Remove metricsPath from ArmeriaSettings - Update Javadoc

Update Javadoc and Documentation

3d6d805

Fix test case

3912cd8

minwoox reviewed Jan 7, 2020

View reviewed changes

Address comments by @minwoox

7413d19

ikhoon added this to the 0.98.0 milestone Jan 9, 2020

trustin approved these changes Jan 10, 2020

View reviewed changes

minwoox approved these changes Jan 10, 2020

View reviewed changes

OneCricketeer approved these changes Jan 10, 2020

View reviewed changes

Address comments by @Cricket007

2f77a76

- Remove unused code - Use MediaType constants for compresssion

trustin merged commit 29c82d8 into line:master Jan 13, 2020

ikhoon deleted the dropwizard-configure branch January 15, 2020 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a consistent configuration to Armeria Dropwizard #2373

Provide a consistent configuration to Armeria Dropwizard #2373

ikhoon commented Jan 4, 2020 •

edited

Loading

trustin commented Jan 6, 2020

trustin commented Jan 6, 2020

ikhoon commented Jan 6, 2020

OneCricketeer left a comment

OneCricketeer commented Jan 6, 2020

ikhoon commented Jan 6, 2020 •

edited

Loading

OneCricketeer commented Jan 6, 2020

ikhoon commented Jan 6, 2020

codecov bot commented Jan 6, 2020 •

edited

Loading

trustin commented Jan 6, 2020

ikhoon commented Jan 6, 2020

minwoox left a comment

OneCricketeer commented Jan 7, 2020

ikhoon commented Jan 7, 2020

trustin left a comment

minwoox left a comment

trustin commented Jan 10, 2020

OneCricketeer left a comment

OneCricketeer Jan 10, 2020

ikhoon Jan 13, 2020 •

edited

Loading

OneCricketeer Jan 13, 2020

OneCricketeer Jan 13, 2020

trustin commented Jan 13, 2020

		@@ -1 +0,0 @@
		com.linecorp.armeria.dropwizard.logging.AccessLogWriterFactory

	/**
	* Configurations for the access log.
	*/
	static class AccessLog {

	/**
	* The access log type that is supposed to be one of
	* {@code "common"} {@code "combined"} or {@code "custom"}.
	*/
	@Nullable
	private String type;

	/**
	* The access log format string.
	*/
	@Nullable
	private String format;

	@Nullable
	String getType() {
	return type;
	}

	void setType(String type) {
	this.type = type;
	}

	@Nullable
	String getFormat() {
	return format;
	}

	void setFormat(String format) {
	this.format = format;
	}
	}
	}

	private static void configureAccessLog(ServerBuilder serverBuilder, AccessLog accessLog) {
	if ("common".equals(accessLog.getType())) {
	serverBuilder.accessLogWriter(AccessLogWriter.common(), true);
	} else if ("combined".equals(accessLog.getType())) {
	serverBuilder.accessLogWriter(AccessLogWriter.combined(), true);
	} else if ("custom".equals(accessLog.getType())) {
	serverBuilder
	.accessLogWriter(AccessLogWriter.custom(accessLog.getFormat()), true);
	}
	}

Provide a consistent configuration to Armeria Dropwizard #2373

Provide a consistent configuration to Armeria Dropwizard #2373

Conversation

ikhoon commented Jan 4, 2020 • edited Loading

trustin commented Jan 6, 2020

trustin commented Jan 6, 2020

ikhoon commented Jan 6, 2020

OneCricketeer left a comment

Choose a reason for hiding this comment

OneCricketeer commented Jan 6, 2020

ikhoon commented Jan 6, 2020 • edited Loading

OneCricketeer commented Jan 6, 2020

ikhoon commented Jan 6, 2020

codecov bot commented Jan 6, 2020 • edited Loading

Codecov Report

trustin commented Jan 6, 2020

ikhoon commented Jan 6, 2020

minwoox left a comment

Choose a reason for hiding this comment

OneCricketeer commented Jan 7, 2020

ikhoon commented Jan 7, 2020

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin commented Jan 10, 2020

OneCricketeer left a comment

Choose a reason for hiding this comment

OneCricketeer Jan 10, 2020

Choose a reason for hiding this comment

ikhoon Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

OneCricketeer Jan 13, 2020

Choose a reason for hiding this comment

OneCricketeer Jan 13, 2020

Choose a reason for hiding this comment

trustin commented Jan 13, 2020

ikhoon commented Jan 4, 2020 •

edited

Loading

ikhoon commented Jan 6, 2020 •

edited

Loading

codecov bot commented Jan 6, 2020 •

edited

Loading

ikhoon Jan 13, 2020 •

edited

Loading