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.

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

[JENKINS-60821] - Let Jetty configure its own defaults #197

Merged
merged 3 commits into from
Jan 22, 2020
Merged

[JENKINS-60821] - Let Jetty configure its own defaults #197

merged 3 commits into from
Jan 22, 2020

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Jan 20, 2020 •
edited
Loading

JENKINS-60821
Jetty knows better than us, so let Jetty configure its own defaults for the ThreadPool (min 8 max 200) and #acceptor/selector threads.

This applies feedback from jetty/jetty.project#4492

Whilst we are here - give all of the Jetty Thread pools different names so it is obvious which pool is which

@jtnord
[JENKINS-60821] let Jetty configure its own defaults
830184e 
Jetty knows better than us, so let Jetty configure its own defaults for
the ThreadPool (min 8 max 200) and #acceptor/selector threads.

This applies feedback from jetty/jetty.project#4492

We loos the thread naming - but all the jetty threads in the servers
where called "Jetty Thread Pool"-something so we did not isolate them
between the reverse proxy and the JenkinsRule or HudsonTestCase so the
fact they are now called qtp-something is pretty much ok.  Nothing else
seems to use Jetty's QueuedThreadPool in Jenkins so any thread starting
qtp is pretty much going to be the jetty thread pool - and it matches
more what you would get when running jenkins with java -jar...
@jtnord jtnord requested review from jglick and MarkEWaite January 20, 2020 18:45
jtnord
jtnord commented Jan 20, 2020
View reviewed changes
src/main/java/org/jvnet/hudson/test/HudsonTestCase.java
server = new Server(new ThreadPoolImpl(new ThreadPoolExecutor(10, 10, 10L, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(),new ThreadFactory() {
public Thread newThread(Runnable r) {
Thread t = new Thread(r);
t.setName("Jetty Thread Pool");
Copy link
Member Author

Choose a reason for hiding this comment

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

we can not apply naming with the QueuedThreadPool without also specifiying the number of threads etc.

@jtnord
More feedback from Jetty
5bdfd1e 
Use a QTP with the defaults and call setName() instead

Uses a unique name per pool so you can identify the various
pools/servers
@jtnord
Copy link
Member Author

jtnord commented Jan 20, 2020 •
edited
Loading

with 5bdfd1e (thanks @sbordet) the threads now look like:

jstack 13944 | grep Jetty
"Jetty Thread Pool (JavaNetReverseProxy)-65" #65 prio=5 os_prio=0 tid=0x000000001fe3c800 nid=0x458c waiting on condition [0x000000002697f000]
"Jetty Thread Pool (JavaNetReverseProxy)-64" #64 prio=5 os_prio=0 tid=0x000000001fe34800 nid=0x1578 waiting on condition [0x000000002687e000]
"Jetty Thread Pool (JavaNetReverseProxy)-63" #63 prio=5 os_prio=0 tid=0x000000001fe33800 nid=0x31cc waiting on condition [0x000000002677e000]
"Jetty Thread Pool (JavaNetReverseProxy)-62-acceptor-0@42e6b13-ServerConnector@66e87363{HTTP/1.1,[http/1.1]}{0.0.0.0:59711}" #62 prio=3 os_prio=-1 tid=0x000000001fe36800 nid=0x3934 runnable [0x000000002667f000]
"Jetty Thread Pool (JavaNetReverseProxy)-61" #61 prio=5 os_prio=0 tid=0x000000001fe3a800 nid=0x2518 runnable [0x000000002657e000]
"Jetty Thread Pool (JavaNetReverseProxy)-60" #60 prio=5 os_prio=0 tid=0x000000001fe38000 nid=0x425c runnable [0x000000002647e000]
"Jetty Thread Pool (JavaNetReverseProxy)-59" #59 prio=5 os_prio=0 tid=0x000000001fe36000 nid=0x2264 runnable [0x000000002637e000]
"Jetty Thread Pool (JavaNetReverseProxy)-58" #58 prio=5 os_prio=0 tid=0x000000001fe33000 nid=0x4ce0 runnable [0x000000002627e000]
"Jetty Thread Pool (JenkinsRule)-22" #22 prio=5 os_prio=0 tid=0x00000000211ea000 nid=0x2310 waiting on condition [0x000000002577f000]
"Jetty Thread Pool (JenkinsRule)-21" #21 prio=5 os_prio=0 tid=0x00000000211e9000 nid=0x3034 waiting on condition [0x000000002567e000]
"Jetty Thread Pool (JenkinsRule)-20" #20 prio=5 os_prio=0 tid=0x00000000211d5000 nid=0x4514 waiting on condition [0x000000002547f000]
"Jetty Thread Pool (JenkinsRule)-19-acceptor-0@7e3d7f8b-ServerConnector@147c7ba8{HTTP/1.1,[http/1.1]}{localhost:59700}" #19 prio=3 os_prio=-1 tid=0x00000000211d4800 nid=0x1c2c runnable [0x000000002537f000]
"Jetty Thread Pool (JenkinsRule)-18" #18 prio=5 os_prio=0 tid=0x00000000211d3800 nid=0x492c runnable [0x000000002501e000]
"Jetty Thread Pool (JenkinsRule)-17" #17 prio=5 os_prio=0 tid=0x00000000211d3000 nid=0x2138 runnable [0x0000000024f1e000]
"Jetty Thread Pool (JenkinsRule)-16" #16 prio=5 os_prio=0 tid=0x00000000211d0000 nid=0x3a04 runnable [0x0000000023bee000]
"Jetty Thread Pool (JenkinsRule)-15" #15 prio=5 os_prio=0 tid=0x0000000020632800 nid=0x1b44 runnable [0x00000000238ae000]

jglick
jglick approved these changes Jan 20, 2020
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Great!

@jtnord jtnord mentioned this pull request Jan 21, 2020
Merged
@jtnord
[JENKINS-60821] remote 'Thread Pool' from the thread pool name
9dd2965 
As pointed out by @jglick it should be obvious from the name that the
thread is in a thread pool (has a #xx after it) - so just remove it.
olamy
olamy approved these changes Jan 21, 2020
View reviewed changes
Copy link
Member

@olamy olamy 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.

definitely a good change!

@MarkEWaite MarkEWaite added the bug label Jan 21, 2020
@MarkEWaite
Copy link
Contributor

Unless there are objections (or someone else merges first), I'll merge this tomorrow.

oleg-nenashev
oleg-nenashev approved these changes Jan 22, 2020
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Fine with me

@MarkEWaite MarkEWaite merged commit 93e9b64 into jenkinsci:master Jan 22, 2020
@jtnord jtnord deleted the jetty-threads branch January 23, 2020 17:01
@oleg-nenashev oleg-nenashev changed the title [JENKINS-60821] let Jetty configure its own defaults [JENKINS-60821] - Let Jetty configure its own defaults Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@MarkEWaite MarkEWaite MarkEWaite approved these changes

@jglick jglick jglick approved these changes

@olamy olamy olamy approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jtnord @MarkEWaite @olamy @jglick @oleg-nenashev