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.

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

[JENKINS-53239] Use default Jetty QueuedThreadPool #54

Merged
merged 3 commits into from Aug 29, 2018

Conversation

Projects
None yet
7 participants
@olamy
Copy link
Member

commented Aug 24, 2018

Jetty QTP per default: It's definitely a well maintained and optimised piece of code.

@olamy olamy force-pushed the olamy:feature/use_standard_jetty_qtp branch 2 times, most recently from d4c1195 to da4f291 Aug 24, 2018

@olamy olamy requested a review from oleg-nenashev Aug 24, 2018

@@ -80,6 +80,10 @@
public static final OInt CONTROL_PORT=integer("controlPort",-1);

public static final OBoolean USE_JMX=bool("useJmx",false);
public static final OBoolean USE_QTP=bool("useQTP",false);

This comment has been minimized.

Copy link
@olamy

olamy Aug 26, 2018

Author Member

@oleg-nenashev I would definitely prefer having true per default here:
public static final OBoolean USE_QTP=bool("useQTP",true);
Well my preferred solution is simply to get RID of the BoundedExecutorService class

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 26, 2018

Member

I am fine with that. CC @jenkinsci/code-reviewers, more votes would be helpful

@olamy olamy force-pushed the olamy:feature/use_standard_jetty_qtp branch from da4f291 to de52837 Aug 26, 2018

[JENKINS-53239] Use default Jetty QueuedThreadPool and get rid of Bou…
…ndedExecutorService

Signed-off-by: olivier lamy <olamy@apache.org>

@olamy olamy force-pushed the olamy:feature/use_standard_jetty_qtp branch from de52837 to 827fd1a Aug 26, 2018

@olamy

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@olamy olamy changed the title use standard jetty qtp (JENKINS-33412, JENKINS-52804, JENKINS-51136) [JENKINS-53239] Use default Jetty QueuedThreadPool Aug 26, 2018

@olamy olamy removed the work-in-progress label Aug 26, 2018

cleanup
Signed-off-by: olivier lamy <olamy@apache.org>
@oleg-nenashev
Copy link
Member

left a comment

You should have probably waited for other comments before removing BoundedExecutorService.

I do not see any code depending on this class, and I doubt it is a compatibility-breaking change. So generally it looks good to me.

It would be great to have a downstream PR to Jenkins which would confirm that the change passes basic tests at least.

@olamy olamy referenced this pull request Aug 27, 2018

Merged

[JENKINS-53239] - Use winstone 5.0 #3602

1 of 4 tasks complete
@oleg-nenashev
Copy link
Member

left a comment

After some consideration, the change looks good to me as is. There is no particular reason to keep a flag for the old behavior, and there should be no compatibility issues. Winstone classes are not exposed to the plugins directly.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@jenkinsci/code-reviewers I would like to land the change in the next weekly and then to consider it for backporting to .2 or .3. It looks reasonable taking the severity of reported issues.

Any feedback is welcome, esp. from @jenkinsci/core

@marco-dev

This comment has been minimized.

Copy link

commented Aug 28, 2018

Change is tested with a provided WAR on Solaris. Without this change the current Solaris versions of Jenkins are not working. Last working version is LTS 2.107.3. So backporting to last LTS would be great.

README.md Outdated
@@ -98,7 +95,10 @@ To run different web applications for diffent virtual hosts:
(e.g., xls=application/vnd.ms-excel:wmf=application/x-msmetafile)
--maxParamCount=N = set the max number of parameters allowed in a form submission to protect
against hash DoS attack (oCERT #2011-003). Default is 10000.
--useJmx = Enable Jetty JMX
--useJmx = Enable Jetty JMX
--qtpMaxThreads = max threads number when using Jetty Queued Thread Pool

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 28, 2018

Member

Maybe it makes sense to explicitly add "Number" to the argument name. Does not matter much tho

This comment has been minimized.

Copy link
@svanoort

svanoort Aug 28, 2018

Member

I think we generally seem to be using "count" - concur that it might make sense to include that, i.e. "qtpMaxThreadCount" and "jettyAcceptorCount" and "jettySelectorCount", etc

But that's more of a style point -- up to the PR author if they want to or not (it's hardly a PR blocker).

This comment has been minimized.

Copy link
@olamy

olamy Aug 28, 2018

Author Member

@svanoort I like this naming proposal! Style it's very important! :)

* and make everyone slow (or worst case choke every request by OOME), so better to err
* on the conservative side (and have inbound connections wait in the queue)
*/
public static final OInt HANDLER_COUNT_MAX =integer("handlerCountMax",40);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 28, 2018

Member

I propose to explicitly add parameter removal in the changelog

--controlPort = set the shutdown/control port. -1 to disable, Default disabled

--handlerCountMax = set the max no of worker threads to allow. Default is 40

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 28, 2018

Member

What will happen if these parameters are defined in startup scripts? IIUC they will be just ignored, right? Even in such case, it makes sense to explicitly document it in the changelog

This comment has been minimized.

Copy link
@olamy

olamy Aug 28, 2018

Author Member

will generate this:

webroot: EnvVars.masterEnvVars.get("JENKINS_HOME")
Exception in thread "main" java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at Main._main(Main.java:344)
	at Main.main(Main.java:160)
Caused by: java.lang.IllegalArgumentException: Unrecognized option: --handlerCountMax-=100
	at winstone.cmdline.CmdLineParser.parse(CmdLineParser.java:52)
	at winstone.Launcher.getArgsFromCommandLine(Launcher.java:356)
	at winstone.Launcher.main(Launcher.java:327)

This comment has been minimized.

Copy link
@olamy

olamy Aug 28, 2018

Author Member

maybe restore them and log a WARNING and sure I will add that to the changelog

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Would it justify Winstone 5.0 BTW? Even if we restore binary compatibility, option removal looks important enough to do that.

@svanoort
Copy link
Member

left a comment

Seems like a solid change - searching is not turning up any uses of BoundedExecutorService either (unsurprisingly).

@dwnusbaum
Copy link
Member

left a comment

Looks good to me, just had some minor questions.

@@ -337,9 +305,6 @@ public void shutdown() {
Logger.log(Logger.INFO, RESOURCES, "Launcher.FailedShutdown", e);
}

// Release all listeners/pools/webapps
this.threadPool.shutdown();

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Aug 28, 2018

Member

Do we still need to clean up the thread pool, or does Jetty handle it now?

This comment has been minimized.

this.threadPool = createThreadPool();
this.server = new Server(new ExecutorThreadPool(threadPool));
int qtpMaxThread = Option.QTP_MAXTHREADS.get(args);
QueuedThreadPool queuedThreadPool = qtpMaxThread>0?new QueuedThreadPool(qtpMaxThread):new QueuedThreadPool();

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Aug 28, 2018

Member

If the option is not specified, then we use an unbounded thread pool. Do we anticipate any problems with that? We could use a default max value instead to avoid unbounded growth.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Aug 28, 2018

Member

@rodrigc Is that the right link? I agree that the change is good and works, I just was curious if there is any benefit to using a max limit to the number of threads even in the default case (e.g. are there any new failure scenarios we could hit if we do not set a max value?):

QueuedThreadPool queuedThreadPool = qtpMaxThread > 0
    ? new QueuedThreadPool(qtpMaxThread)
    : new QueuedThreadPool($SOME_DEFAULT_HERE); // Would it be better to put a (large) max here instead of making it unbounded?

This comment has been minimized.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Aug 28, 2018

Member

Thanks @olamy looks like I was just confused!

@skeenan947

This comment has been minimized.

Copy link

commented Aug 28, 2018

I just tested this on a 72-core amd64 k8s node and it worked perfectly. Published a docker image built off of this (thanks to @olamy for building the war) at skeenan947/jenkinstest
The sooner we can get this into an LTS version of Jenkins, the better - a bunch of us are stuck at 2.107.3 until this is in.

@olamy

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

+1 for 5.0 version I will update that and core pr as well.

changes after review
Signed-off-by: olivier lamy <olamy@apache.org>
@olamy

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

Taking the feedback, I will just merge it and ship so that it lands in Incrementals early

@oleg-nenashev oleg-nenashev merged commit 74775cc into jenkinsci:master Aug 29, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

P.S: I have no idea whether we can backport that. Will need a discussion

@olamy olamy deleted the olamy:feature/use_standard_jetty_qtp branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.