Add 'threads' configuration group for embedded containers #19475
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| @DeprecatedConfigurationProperty(replacement = "server.jetty.acceptors") | ||
| public Integer getAcceptors() { | ||
| return this.acceptors; | ||
| return this.getThreads().getAcceptors(); |
There was a problem hiding this comment.
A bit of inconsistency on naming and plurality between Jetty#acceptors/selectors and Undertow#io/worker. Should we
a) move acceptors -> acceptor
b) move io -> ios
c) include num- prefix in all of them?
d) leave alone - not a big deal
There was a problem hiding this comment.
Thanks a lot for the PR @Bono007. It's quite complete and looking at the proposal, I am wondering if threads is the best name for the group. I've added a few comments and would like to hear from the team before going forward with this one.
| /** | ||
| * Thread related configuration. | ||
| */ | ||
| private final Threads threads = new Threads(); |
There was a problem hiding this comment.
Looking at the code I am wondering now if pool wouldn't be better and more consistent with other properties of the same nature.
@Bono007 no need to act on that now, I'd like to get some more feedback before asking you to change anything.
There was a problem hiding this comment.
Its interesting because both concepts are related but not exclusive of one another. Some servers may not have pool properties, some may have thread properties that are unrelated to pool, etc.
I think threads is more generic than pool and most developers will associate that w/ the current thread related properties they are used to.
There was a problem hiding this comment.
After thinking about this some more, suppose I were to add another maxQueueDepth property to Jetty, this would not make sense under threads but would make sense under pool.
| } | ||
|
|
||
| @Deprecated | ||
| @DeprecatedConfigurationProperty(replacement = "server.jetty.acceptors") |
There was a problem hiding this comment.
That replacement value is wrong.
There was a problem hiding this comment.
Good catch. Updated.
@snicoll Boot MR question... Should I rebase and squash subsequent commits to MR based on feedback (like this case) or just add more commits and let Github squash at end of MR?
There was a problem hiding this comment.
Not a single answer here. When it's about fixing a PR review (as you did), we prefer a single squashed commit rebased on top of the latest state of the target branch (here master). When there is a discussion and you'd like to suggest something, a separate commit is better in case we don't pursue with the suggestion.
| } | ||
|
|
||
| @Deprecated | ||
| @DeprecatedConfigurationProperty(replacement = "server.jetty.selectors") |
There was a problem hiding this comment.
That replacement value is wrong.
| * Number of I/O threads to create for the worker. The default is derived from | ||
| * the number of available processors. | ||
| */ | ||
| private Integer io; |
There was a problem hiding this comment.
io is a bit odd on its own. I don't have bright ideas but perhaps we should add something in the property name that better conveys it's a number of threads for a certain "type" of threads.
There was a problem hiding this comment.
Yep I struggled w/ this balance of brevity and better conveyed meaning. For this one I was thinking io-per-worker as that is actually what it is.
snicoll
added
type: enhancement
philwebb
removed
the
for: team-attention
|
@Bono007 now that your other PR has been merged, would you be willing to rebase your proposal and include the |
snicoll
added
the
status: waiting-for-feedback
|
@snicoll Absolutely. I will do that this evening. MR ready sometime in the next 24
hours.
If you need it before then let me know and I will make the time.
…On Fri, Feb 14, 2020, 7:59 AM Stéphane Nicoll ***@***.***> wrote:
@Bono007 <https://github.com/bono007> now that your other PRs has been
merged, would you be willing to rebase your proposal and include the
max-queue-capacity property that has been introduced for Jetty?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19475?email_source=notifications&email_token=AG4RTQ5JJZL6QKIWCS3QJJTRC2PULA5CNFSM4KANTAUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELZDKQI#issuecomment-586298689>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG4RTQ37LA7G7CZXM7FCUQDRC2PULANCNFSM4KANTAUA>
.
|
spring-projects-issues
added
status: feedback-provided
| assertThat(this.properties.getJetty().getThreads().getMaxQueueCapacity()).isEqualTo(5150); | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
Has this been released anywhere yet? If not, I can remove it - just let me know.
|
@snicoll there were a few comments in the original proposal that we never resolved. I am ok with the proposal as is - if you want anything changed just let me know. |
snicoll
changed the title
|
Thanks again @Bono007 |
Moves all thread related server properties under a "threads" group for each server implementation that has thread related properties (
tomcat|jetty|undertow).The existing property accessors have been marked as deprecated. The old impl just delegates to the new one which allows us to test that fact in a deprecated test and then all subsequent downstream tests can rely on the new api (ie have deprecate test in a single place for each property).
@snicoll do the Boot docs get auto-generated from the
@DeprecatedConfigurationPropertys?Closes gh-18620