Skip to content

Provide a configuration property for configuring Tomcat's processor cache #15054

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Provide a configuration property for configuring Tomcat's processor cache #15054

wants to merge 6 commits into from

Conversation

ayudovin
Copy link
Contributor

@ayudovin ayudovin commented Nov 1, 2018

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 1, 2018
snicoll
snicoll requested changes Nov 1, 2018
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR. I've added a few suggestions to make our code style more explicit.

Please be aware that master has not switched to 2.2.x yet so we can't really merge this PR at the moment.

...autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java Outdated
@@ -381,6 +382,19 @@ public Session getSession() {
*/
private final Resource resource = new Resource();

/**
* Configuring processor cache.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please polish the property description?

...autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java Outdated
/**
* Configuring processor cache.
*/
private int processorCache = 200;
Copy link
Member

Choose a reason for hiding this comment

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

We try to order things a bit more naturally. This property could be moved below acceptCount.

...autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java Outdated
*/
private int processorCache = 200;

public int getProcessorCache() {
Copy link
Member

Choose a reason for hiding this comment

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

The getter/setter pair should be added in the right order (after the one for acceptCount) rather than adding them where they are now.

...g/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizerTests.java Outdated
@@ -280,6 +280,12 @@ public void testCustomizeMinSpareThreads() {
assertThat(this.serverProperties.getTomcat().getMinSpareThreads()).isEqualTo(10);
}

@Test
Copy link
Member

Choose a reason for hiding this comment

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

Ditto. Please add this test after customAcceptCount

spring-boot-project/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc Outdated
@@ -284,6 +284,7 @@ content into your application. Rather, pick only the properties that you need.
server.tomcat.resource.cache-ttl= # Time-to-live of the static resource cache.
server.tomcat.uri-encoding=UTF-8 # Character encoding to use to decode the URI.
server.tomcat.use-relative-redirects= # Whether HTTP 1.1 and later location headers generated by a call to sendRedirect will use relative or absolute redirects.
server.tomcat.processor-cache= # Custom value for Tomcat's processor cache.
Copy link
Member

Choose a reason for hiding this comment

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

Properties are ordered in natural order. This property must be moved.

Also, the field description must match the description here.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Nov 1, 2018
@ayudovin
Copy link
Contributor Author

ayudovin commented Nov 1, 2018

@snicoll, thank you for your suggestion, I have updated pull request this suggestions.

snicoll
snicoll reviewed Nov 1, 2018
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the quick feedback. Unfortunately, we're not there yet, I've added two suggestions.

...autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java Outdated
@@ -369,6 +370,15 @@ public Session getSession() {
*/
private int acceptCount = 100;

/**
* The maximum number of idle processors that will be retained in the cache and
Copy link
Member

Choose a reason for hiding this comment

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

Thank you but we're not there yet. Could you please look at other keys in ServerProperties for inspiration? Key description does not start with a or the and can't contain javadoc tags as these are not processed by the annotation processed. We don't document default value either as the value is already set there and made available in the metadata.

Looking at your update, this looks ideal to me:

Maximum number of idle processors that will be retained in the cache and reused with a subsequent request.

Please note that key descriptions are not meant to copy/paste the official documentation of the thing it's configuring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thx for advice

spring-boot-project/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc Outdated
@@ -276,6 +276,7 @@ content into your application. Rather, pick only the properties that you need.
server.tomcat.max-threads=200 # Maximum amount of worker threads.
server.tomcat.min-spare-threads=10 # Minimum amount of worker threads.
server.tomcat.port-header=X-Forwarded-Port # Name of the HTTP header used to override the original port value.
server.tomcat.processor-cache= # The maximum number of idle processors that will be retained in the cache and re-used with a subsequent request. The default is 200. A value of -1 means unlimited.
Copy link
Member

Choose a reason for hiding this comment

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

This is still a different description than the field javadoc. As indicated previously, they must be identical.

(When a description is a bit more lengthy than usual, we take the first sentence only but that's not the case here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thx for advice

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the super quick update on the suggestions!

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Nov 1, 2018
@snicoll snicoll added this to the 2.2.x milestone Nov 1, 2018
@snicoll snicoll mentioned this pull request Nov 1, 2018
Closed
@snicoll snicoll self-assigned this Dec 3, 2018
@snicoll snicoll modified the milestones: 2.2.x, 2.2.0.M1 Dec 3, 2018
snicoll pushed a commit that referenced this pull request Dec 3, 2018
@ayudovin @snicoll
Add configuration property for configuring Tomcat's processor cache
3059f0e 
See gh-15054
@snicoll snicoll closed this in 1b40b0e Dec 3, 2018
snicoll added a commit that referenced this pull request Dec 3, 2018
@snicoll
Merge pull request #15054 from ayudovin
2ac76b3 
* pr/15054:
  Polish contribution
  Add configuration property for configuring Tomcat's processor cache
@snicoll
Copy link
Member

snicoll commented Dec 3, 2018

Thanks @ayudovin, I've merged your contribution with a polish commit.

@ayudovin
Copy link
Contributor Author

ayudovin commented Dec 3, 2018

@snicoll, thank you!

@wilkinsona wilkinsona mentioned this pull request Apr 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll approved these changes

Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.2.0.M1
Development

Successfully merging this pull request may close these issues.
3 participants
@ayudovin @snicoll @spring-projects-issues