Add more configuration properties to TomcatService #96
Conversation
|
@yfinkelstein Could you please take a look and let us know if this solves #83? |
trustin
force-pushed
the
more_tomcat_options
branch
from
f4ad5d4
to
1d05ac5
Compare
|
For reference, here's spring-boot's embedded tomcat configuration interface: Seems to provide customization of stuff like Valve, LifecycleListener, Context, Connector. I'm not experienced enough in tomcat to know what out of these is actually important to have, and we should start with basics, but it's probably a good reference point for what fully featured embedded tomcat may involve. |
| */ | ||
| public TomcatServiceBuilder configurator(Consumer<? super StandardServer> configurator) { | ||
| @SuppressWarnings("unchecked") | ||
| final Consumer<StandardServer> castConfigurator = |
There was a problem hiding this comment.
Can't you make the type of the of the field <? extends StandardServer> to avoid the casting?
There was a problem hiding this comment.
The use of super is intentional because a configurator might expect Server or LifeCycleMBean. It seems like I can remove this unchecked cast simply by changing the type of TomcatServiceConfig.configurators to List<? super StandardServer>, so let me just fix it that way.
| * the Tomcat {@link StandardServer} created by a {@link TomcatService}. This method can be invoked | ||
| * multiple times to add multiple {@code configurator}s. | ||
| */ | ||
| public TomcatServiceBuilder configurator(Consumer<? super StandardServer> configurator) { |
There was a problem hiding this comment.
configurator can be used to add/remove services to TomcatService. is it okay without any restriction?
There was a problem hiding this comment.
Sounds like a valid concern. Let me add some sanity checks to TomcatServiceInvocationHandler.start() and add its test.
There was a problem hiding this comment.
Here's the list of the properties I will check. Let me know if you think there should be more:
// 1. Check if the default connector has not been removed
// 2. Check if the default service has not been removed
// 3. Check if a new service has not been added
// 4. Check if engine has not been changed
// 5. Check if baseDir has not been changed
// 6. Check if Server.port has not been changed
// 7. Check if the default host and context have not been removedThere was a problem hiding this comment.
Even though my personal preference is to whitelist available configurations, but anyway it looks good enough.
trustin
force-pushed
the
more_tomcat_options
branch
from
1d05ac5
to
07b48df
Compare
|
Other code looks good to go! let me know whether sanity check will be done in separate PR or this PR. |
|
@blmarket Will address the sanity check in this PR and ping you. Thanks for a review! |
trustin
force-pushed
the
more_tomcat_options
branch
from
07b48df
to
553d43d
Compare
|
@blmarket PR updated. Please take a look at |
Related: line#83 Motivation: Tomcat registers JMX MBeans using the configured service name, which is often 'Catalina' as written in the default Tomcat server.xml file. TomcatService currently does not use 'Catalina' but 'Tomcat-over-Armeria' as the default service name. It makes the legacy JMX clients confused because they assume a Tomcat MBean always has the name 'Catalina'. Also, a user might want to set some additional properties of Tomcat which might not be used by an ordinary web applications. e.g. Service.addExecutor(). Modifications: - Add serviceName() and engineName() property - Change the default service/engine name to 'Catalina' which is the default in conf/server.xml - Add configurator() builder operation so that a user can configure the server more freely - Add configuration sanity checks after the user-specified configurators are run to prohibit the modification of the properties that are supposed to be configured via the builder properties. - Miscellaneous: - Upgrade Tomcat to 8.0.30 Result: - A user has pretty much full control over the embedded Tomcat configuration
|
test failed, but it passes at local machine. is it JDK-related issue? |
|
@blmarket Yes, TravisCI sometimes uses very old JDK 8 version with a compiler bug. |
|
@trustin Thanks for explanation! I'm merging it 😄 |
Add more configuration properties to TomcatService
|
@blmarket Thanks! :-) |
Related: #83
Motivation:
Tomcat registers JMX MBeans using the configured service name, which is
often 'Catalina' as written in the default Tomcat server.xml file.
TomcatService currently does not use 'Catalina' but
'Tomcat-over-Armeria' as the default service name. It makes the legacy
JMX clients confused because they assume a Tomcat MBean always has the
name 'Catalina'.
Also, a user might want to set some additional properties of Tomcat
which might not be used by an ordinary web applications. e.g.
Service.addExecutor().
Modifications:
default in conf/server.xml
server more freely
Result:
configuration